torcontrol: Define tor reply code as const to improve our maintainability #32166

nit: Please use the same comment syntax as the other comments

Thank you, fixed.

It's strange that the only torproject.org link for the C Tor manual is a 2019 backup of the site, but haven't found anything better either. The only up-to-date canonical source is the manual page, which isn't particularly readable on the web https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt (nothing to do here really, just noting)

Yes the web url link is old, but the IsolateSOCKSAuth explaination content is same with https://gitlab.torproject.org/tpo/core/tor/-/blob/main/doc/man/tor.1.txt

Oh yes the site is correct, i'm just a bit worried about it going offline at some point, so was trying to find a more up-to-date official link. But it doesn't exist.

nit: Others might disagree but I prefer grouping with an enum:

enum {
    TOR_REPLY_OK = 250,
    TOR_REPLY_UNRECOGNIZED = 510,
};

(Developer-notes prefer enum class but I think implicit conversion to int is more important in this case, and there is no risk of forgetting the non-existent enum name).

These constants are hardcoded in src/test/fuzz/torcontrol.cpp as well, but I think it's okay to let that be.

If you decide to not convert them to enum, it would be nice to use constexpr on these new lines.

I'm fine with either option; let the Bitcoin maintainers decide.

i considered commenting this, but didn't because it's a bit much for defining two response values. Seems the benefit of enum class would be to make sure they're grouped by name TorReply::OK TorReply::UNRECOGNIZED?

Seems the benefit of enum class would be to make sure they're grouped by name TorReply::OK TorReply::UNRECOGNIZED?

Yes, but then one would have to do if (foo->bar == static_cast<int>(TorReply::OK)) which becomes a bit verbose.

Oh, right, and enums are not good for types that might have values that are not specifically defined, might as well keep this as-is.

informational, related:

Still feels scary to me how _randomize_credentials=true leads to a predictably incrementing integer to be used for user/pass after 5eaaa83ac1f5eb525f93e2808fafd73f5ed97013. What if guarantees of the Tor API were misunderstood, or change in the future?

Guess it's very unlikely to be a problem if these are only used to authenticate with the local SOCKS5 proxy and don't reach any further.

nit: Think clang-tidy will verify the name matches if you append = to the comment.

    Proxy addrOnion = Proxy(resolved, /*_randomize_credentials=*/ true);

Thank you.

The requirements for stream isolation are documented here: https://spec.torproject.org/socks-extensions.html#stream-isolation . Our case is "They are both SOCKS5 with USERNAME/PASSWORD authentication, using legacy isolation parameters, and they have identical usernames and identical passwords". The <torS0X> extension is very recent so we don't use that (yet?).

So using different credentials is enough, there is no requirement, nor even benefit for them to be random.

Although thinking of it, it's possible to think of edge cases:

• if you restart the daemon in quick succession, the counter resets to 0 so a circuit may be reused
• multiple bitcoinds running in parallel might use each other's circuits

So maybe it's worth doing something else here, for example generate a unique prefix per bitcoind session startup (this would be once, so no need for performant randomness). But it's out of scope for this PR.

Pretty sure it is was already validated, this is how we discovered that the parameter is _randomize_credentials not randomize_credentials (https://github.com/bitcoin/bitcoin/pull/31973#discussion_r1977739770).

Oh, we used to have a = but it was lost? You're right.

So maybe it's worth doing something else here, for example generate a unique prefix per bitcoind session startup (this would be once, so no need for performant randomness). But it's out of scope for this PR.

i'll open a PR for this. i am also going to rename the parameter to tor_stream_isolation to make it more specific.

Thanks! Happy I managed to prompt you to come up with those edge-cases.