💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011041024)
(Inline thread in random position)

nit: src/external_signer.h references "test" and never "testnet4" which seems to be the argument the code would be sending in.

👍 hodlinator approved a pull request: "Move some tests and documentation from testnet3 to testnet4"
(https://github.com/bitcoin/bitcoin/pull/32096#pullrequestreview-2711851156)
ACK 2906b183169bc78b37449a818717249c2d1cb7a1

Ran functional tests with `--extended` with on a non-multiprocess `devmode` build (in part to verify network `MAGIC_BYTES` change).

💬 hodlinator commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011028887)
Should be removed as these tests currently duplicate the L669-670. Don't think those checks mean to be dependent on surrounding lines. Probably good to keep L685+L687 though.

💬 hodlinator commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011039690)
(Inline thread in random position)

informational: https://developer.bitcoin.org/examples/intro.html / https://github.com/bitcoin-dot-org/developer.bitcoin.org/blob/master/examples/intro.rst
still references the Testnet3 RPC port 18332. But the repo hasn't been updated for 4 years.

💬 chellas2wp commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2011428282)
Starting process/approval request/apply specifications.

💬 davidgumberg commented on issue "wallet: migratewallet crashes "Assertion `legacy_spkm' failed"":
(https://github.com/bitcoin/bitcoin/issues/32112#issuecomment-2750254947)
Just to add context, the crash is on an [`Assume()`](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/wallet.cpp#L4087) so it can't happen in a production build, where an unhelpful [error message is printed](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/wallet.cpp#L4089) when this happens.

This should be caught by `BerkeleyRODatabase::Open()` which makes [efforts](https://github.com/bitcoin/bitcoin/blob/1
...

💬 maflcko commented on pull request "test: replace hardcoded fee with node relay fee based calculation":
(https://github.com/bitcoin/bitcoin/pull/32058#discussion_r2011486944)
Many other tests do it the same way. See for example `test/functional/p2p_1p1c_network.py`.

If you wanted to change that, you'd have to go through all tests and check them for applicable changes, but I don't really see the point to do that without rationale.

👍 rkrux approved a pull request: "doc: clarify the documentation of `Assume` assertion"
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2712716044)
re-ACK 329a0dcdafe05002f662e8737a76bfdeaba9a3ed

💬 davidgumberg commented on issue "bitcoind crash with corrupt wallet.dat":
(https://github.com/bitcoin/bitcoin/issues/32124#issuecomment-2750380771)
This specific crash occurs after an unhandled [exception is thrown](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/sqlite.cpp#L162-L165) in `SQLiteBatch::SetupSQLStatements()`, when creating a `WalletBatch()` [during](https://github.com/bitcoin/bitcoin/blob/1d281daf8613a3cb7a26759c351ffb34dab0f656/src/wallet/wallet.cpp#L2389) `LoadWallet()`.

IIUC, none of `SQLiteBatch()`'s users, handle errors thrown during batch creation.

📝 hodlinator opened a pull request: "descriptors: Multipath/PR 22838 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/32134)
Follows up on unresolved suggestions from #22838. In order of priority:

1. Fixes a couple of typos [^1][^2] and indentation to conform to Markdown.
2. Solves `for`-loop nit [^3] and also limits variable scope.
3. Clarifies data relationships [^4][^5] by introducing `struct` rather than comments.

[^1]: https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1713711352
[^2]: https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1735039600
[^3]: https://github.com/bitcoin/bitcoin/pu
...

💬 hodlinator commented on pull request "descriptors: Multipath/PR 22838 follow-ups":
(https://github.com/bitcoin/bitcoin/pull/32134#issuecomment-2750443435)
CC @glozow & @jonatack since these follow-ups are mostly based on your comments in the original PR (see references in PR description).

I ran into the broken formatting and 2/3 typo while looking at another PR and traced them back to 22838. Don't feel strongly about the two refactoring commits. Fine with dropping either/both to not hold the doc-commit hostage.

💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011552811)
This lists all the options available to the user, so I think it belongs here.

I'll look into your suggestion on the next rebase.

💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011555131)
Indeed this should move.

(it's handy if you mention the commit when making a comment)

💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011562073)
I assume you're talking about the `chain` argument? This is passed directly to HWI (or equivalent software). I'll update the comment on the next rebase to swap "test" for "testnet4" which was missed when testnet4 was launched.

💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011563866)
I found myself wondering what ports to use on mainnet, so I'll probably just leave it as testnet4.

💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#discussion_r2011566712)
The `bitcoin.org` is independent from Bitcoin Core, but you could open a PR there to recommend that they recommend testnet4.

💬 maflcko commented on issue "Failed transactions on importing mempool":
(https://github.com/bitcoin/bitcoin/issues/32125#issuecomment-2750489528)
You can modify the source code in `src/node/mempool_persist.cpp` to print the transaction ids (and error reasons) of the failed imports. Then call `getmempoolentry` to get the mempool topology and fees after your successful import to debug this further.

💬 maflcko commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#discussion_r2011592895)
thx, done. Should be trivial to re-review

👍 hebasto approved a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2712884249)
re-ACK 77e553ab6a0a98d065884a83490f28eec6df0e23.

👍 laanwj approved a pull request: "build: Drop option to disable hardening."
(https://github.com/bitcoin/bitcoin/pull/32071#pullrequestreview-2712899044)
re-ACK 77e553ab6a0a98d065884a83490f28eec6df0e23