Wouldn't it be simpler to do something like:

class MapOpNames : public std::map<std::string, opcodetype>
{
public:
    MapOpNames()
    {
        for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
            ...
        }
    }
};
opcodetype ParseOpCode(const std::string& s)
{
    const static MapOpNames mapOpNames;
    auto it = mapOpNames.find(s);
    ...
}

?

maybe just move map initialization out of ParseOpCode?

static std::map<std::string, opcodetype> get_map()
{
    std::map<std::string, opcodetype> map;
    for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
          ...    
    }
    return map;
};

opcodetype ParseOpCode(const std::string& s)
{
    const static std::map<std::string, opcodetype> mapOpNames(get_map());
    auto it = mapOpNames.find(s);
    ...
}

Those are both solid suggestions, I guess I wasn't familiar enough with initializer rules to see those would be safe. Will probably go with @kirillkovalenko's approach

Although thinking about it it's not clear to me that in your version get_map isn't called every time the function is run?

Why would it? You can test yourself:

#include <iostream>

static int get_map() {
  std::cout << __func__ << std::endl;
  return 42;
};

bool ParseOpCode() {
  const static auto mapOpNames{get_map()};
  return mapOpNames == 1;
}

int main() {
  ParseOpCode();
  ParseOpCode();
}

Although thinking about it it's not clear to me that in your version get_map isn't called every time the function is run?

It's the way language/runtime implements it, basically a guarantee
https://en.cppreference.com/w/cpp/language/storage_duration#Static_local_variables

There are issues in other parts of the codebase if this assumption doesn't hold

reading the spec more closely I think it should be ok

I think you could also do:

namespace {
class OpCodeParser
{
private:
    std::map<std::string, opcodetype> mapOpNames;

public:
    OpCodeParser()
    {
        for (unsigned int op = 0; op <= MAX_OPCODE; op++) {
            ...
        }
    }
    opcodetype Parse(const std::string& s) const {
        auto it = mapOpNames.find(s);
        ...
    }
};    
} // namespace

opcodetype ParseOpCode(const std::string& s)
{
    const static OpCodeParser ocp;
    return ocp.Parse(s);
}

went with @ajtowns suggested edit.

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

Are you still working on this? If not, I suggest to close this PR

practicalswift --> are there tools we can deploy to catch usage like this?

bitcoin-tx is single threaded. I don't think tooling exists to catch "races" in single-threaded programs, whatever that means.

well i think core_read is still included in libbitcoin sources so i think it should be threadsafe -- good rule of thumb is that any pure function should be threadsafe, always.

i don't think closing the PR is a good idea; i'll try to rebase it when i have time.

good rule of thumb is that any pure function should be threadsafe, always.

There's some risk that this function gets moved to an utility at some point when it is needed somewhere else, and people forget to make it thread-safe.

I'm fine with making this change, but please address the comments.

rebased, nits addressed

review ACK cfa2b8dbf9fc6d020281e6521e83853838f62f81 🗡

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

review ACK cfa2b8dbf9fc6d020281e6521e83853838f62f81 🗡
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUie4gv/ZLgNcH13qKel1nfbVmVAe4FRMQ5/D76URcNUf6jNb0xbgcSloTRhCx+j
d4aFKjPrCpo2qzpRO3i7llmM3HND48EQuMM2he2dKqZVCfh2q5I8YZcAokBPpsw3
5g/4LrPaKTcaINwD42txaJX6yNG0oTuM2xabTatfeAq310GMSw0mDqKCBNkw+9vO
N34Un7/mWR+zv1EQLqGI2SsJiYoI7MQjh8dH9e35gE19Z+fsthebEsccSy+EcbYq
ZGQZiANnmvGDUzExjg0/6isxmwf8uJBfDFztKPuSV+FY0jG2oT9scS8B8p9H1q88
zFxt02MJfUegj0i6O3UKp4L5qJnvICEIdfDIUyKBTENYj/hLiw5gbjPvJABhn+lW
Ef6NArWBrSPGrb15pGk8CjwefZIAW+vgNyQ/EUR3ihPnXssO/8i4Fe75ic9Ib6Xy
7p/l/ZhH8noG1J8zFMe5OnLKeFAmZNMg3q+zenHkacmI9V24ttV1+xwWmuWmf4wb
+yXKFOKH
=Q+n8
-----END PGP SIGNATURE-----

@JeremyRubin would be really nice to fix the two nits, but this can be also be merged as-is. Please let us know what you prefer.

since the current PR has 2 code review ACKs on cfa2b8d, i don't want to burden anyone with re-review on that hash, so I pushed a fixup with it. If you want i can squash it, or you can on merge.

Thank you for the fixup. Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

maintainers aren't allowed to squash, as it would break the signature check script

squashed