💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1834010212)
Marking this as resolved because I removed the requirement to set `-walletbroadcast=0` and extended the `-privatebroadcast` doc.

> My understanding was that anything done via the wallet would work as usual, but that transactions sent via sendrawtransaction would use the private broadcast mechanism if set

Yes, this is exactly how it works now.

💬 rkrux commented on pull request "Update manpage descriptions":
(https://github.com/bitcoin/bitcoin/pull/29686#issuecomment-2464236232)
> > I just realised it would be great to commit the newly generated man pages here? https://github.com/bitcoin/bitcoin/tree/master/doc/man
> > There are already outdated man pages in that directory, might as well update them now.
>
> I don't understand. How are the placeholders outdated and how should they be updated?

Oh my bad, I forgot they are placeholders.

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2464245393)
Yeah, don't worry about the ctest bug. It is unrelated and should be fixed somewhere else. I was mostly wondering why a force push that does not change behavior (https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680) suddenly makes the CI trip. Why would CI pass before and not after, when `-testdatadir` isn't set. Maybe someone can run the two commits locally on Windows?

🤔 maflcko reviewed a pull request: "test: Rework wallet_migration.py to use previous releases"
(https://github.com/bitcoin/bitcoin/pull/31248#pullrequestreview-2423261020)
left a question/nit

💬 maflcko commented on pull request "test: Rework wallet_migration.py to use previous releases":
(https://github.com/bitcoin/bitcoin/pull/31248#discussion_r1834052007)
I wonder if this diff can be removed by just using 28 as the previous release, with the `-deprecatedrpc=create_bdb` option enabled?

💬 maflcko commented on pull request "test: Add combinerawtransaction test to rpc_createmultisig":
(https://github.com/bitcoin/bitcoin/pull/31249#issuecomment-2464306592)
Forgot to move ab98e6fd039 as well? Otherwise

lgtm ACK af4d23178b420f46196fbace2176ce1fe94ed9cd

📝 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. :)