📝 rkrux opened a pull request: "contrib: correct default build path in man pages"
(https://github.com/bitcoin/bitcoin/pull/31255)
The `gen-manpages.py` script is unable to find
the executables because it tries to search in
`/bitcoin/src` when no custom `BUILDDIR` is set.
With the new build process in place, the default
build dir is now set to `/bitcoin/build`.

Prior to this change, the script stopped with a
`not found or not an executable` error. Now it
generates the man pages without needing to
explicitly set `BUILDDIR` variable.

<!--
*** Please remove the following help text before submitting: ***

Pull r
...

✅ fanquake closed a pull request: "contrib: correct default build path in man pages"
(https://github.com/bitcoin/bitcoin/pull/31255)

💬 fanquake commented on pull request "contrib: correct default build path in man pages":
(https://github.com/bitcoin/bitcoin/pull/31255#issuecomment-2464326154)
Thanks, however this is part of #30986.

💬 rkrux commented on pull request "contrib: correct default build path in man pages":
(https://github.com/bitcoin/bitcoin/pull/31255#issuecomment-2464346400)
> Thanks, however this is part of #30986.

Oh I see, I will take a look at it, thank you.

:lock: fanquake locked an issue: "Authentication with KYC"
(https://github.com/bitcoin/bitcoin/issues/31217)

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834123117)
sdfsdf

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834128269)
Wanted to make clear it wasn't something internal to `BitcoinTestFramework/TestNode` deciding to set the RPC timeout. Replaced that with instead appending ", expect one warning" to the log message.

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834132074)
Goot point, changed to
`Suppress these in favor of a later outcome (success, FailedToStartError, or timeout).`

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834132851)
Made comment less definitive.

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834133495)
Added:
`Suppress similarly to the above JSONRPCException errors:`

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834134904)
Agree, made comment less definitive, but still think it's good to include the common cause:
`Suppress if cookie file is missing and no rpcuser or rpcpassword; bitcoind may be starting`

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2464405109)
Thanks for the reviews! Was trying to keep comments largely intact, but agree with @maflcko that they were somewhat incorrect, so altered them. Also renamed `ignored_errors` (introduced by this PR) to `suppressed_errors` since it felt more fitting and also matches the updated comments.

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1834151211)
I don't think this is correct. The size is identical for both, so how can it be invalid for one, but not the other? They are invalid due to the embedded `=` and `\0`.

💬 rkrux commented on pull request "doc: recitfy typos":
(https://github.com/bitcoin/bitcoin/pull/31253#issuecomment-2464434603)
> If your change contains a merge commit, the above workflow may not work and you will need to remove the merge commit first. See the next section for details on how to rebase.

The merge commit would need to be removed: https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#rebasing-changes

> doc: recitfy typos

Also, there's a typo in the PR title. :)

💬 maflcko commented on pull request "test: Introduce ensure_for helper":
(https://github.com/bitcoin/bitcoin/pull/30893#discussion_r1834183665)
f04529fc40f2f7c8e4e63d892cdcbe6c91dfbf96: Any reason to make this a member method, when it doesn't use any member fields? I'd say it would be fine to either make this a global, standalone, util function. Otherwise, if you want to make it a member, it would be good to also add it to the other classes, like test_framework. Then, always use it from the outer-most scope.

For example, it is a bit confusing to call `self.nodes[0].p2ps[0].ensure_for(...)`, when `self.ensure_for(...)` is identical, o
...

💬 maflcko commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1834202940)
Is it really useful to map one exception type to another for a test-only dev-only debug-only feature? I'd say it seems fine to just use the python built-in exceptions, which should be self-explanatory. I understand that they won't print the method name, but it could make sense to log that either way as the first thing in the for loop?

💬 maflcko commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-2464504665)
I think fixing stability would be nice, but not a blocker

👍 rkrux approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2423578275)
Coming here from https://github.com/bitcoin/bitcoin/pull/31255, putting my two cents in.

I had to generate man pages while reviewing a PR and found that the script threw error even in the default case due to the new build process that moved the binaries in `/build`, so a strong ACK for 0194a638d31fef7e8193c50843c11d2c2ee212a3 because IMO the script should work in the default case w/o needing to set the `BUILDDDIR`.

As I read from @laanwj's [comment](https://github.com/bitcoin/bitcoin/pull/
...

💬 rkrux commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1834300124)
```
Set BUILDDIR to specify the correct build path.
```

Is the intent to make the user explicitly set BUILDDIR? Not aware of the usual use cases but as a user, my preference is to just run the script to generate the man pages w/o needing to set an env var. I'd be ok with this error as well.

```
print(f'No binaries found in {builddir}. Please specify the correct build path.')
```

💬 rkrux commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#discussion_r1834261233)
nit: for verbosity
```
help="skip generation for binaries that are not found in the build path",
```