💬 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

💬 Sjors commented on pull request "Move some tests and documentation from testnet3 to testnet4":
(https://github.com/bitcoin/bitcoin/pull/32096#issuecomment-2750558547)
Rebased (just in case), added testnet4 magic bytes coverage to `feature_assumeutxo.py` (new commit), removed the duplication in `argsman_tests.cpp`.

💬 maflcko commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2750566651)
Also, for libc++ you are enabling the strongest non-abi breaking one, so it would be better to do the same for gcc, which probably is `_GLIBCXX_DEBUG_PEDANTIC`?

💬 maflcko commented on pull request "wallet, rpc: deprecate settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r2011661090)
it doesn't matter right now, but this is too strict. `skip_test_if_missing_module` will skip the whole test when the wallet is missing, even if a part of the test is non-wallet related.

It would be better to just guard the specific test case that needs a wallet, so that newly added non-wallet test cases can be run without the wallet.

💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2011662736)
> I tried to use names similar to BIP-345

The terms `{vault,trigger,withdrawal,recovery} transaction` from that BIP are indeed useful. But I found it a bit easier to follow when calling the relevant keys "hot" and "cold" and not having a third one.

> You could, but I don't think that makes a lot of sense in practice, as the alternate_pk has no covenant restriction.

Maybe I'm still confused then. In my branch I renamed `recover_pk` to `cold_pk`. So the cold key can be used for keypath s
...

💬 hodlinator commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#discussion_r2011687872)
Exactly, from what I could see it would be set from `ArgsManager::GetChainTypeString()` in one case and `CChainParams::GetChainTypeString()` in another.

⚠️ maflcko opened an issue: "fuzz: psbt_base64_decode fails on Windows"
(https://github.com/bitcoin/bitcoin/issues/32135)
https://github.com/bitcoin/bitcoin/actions/runs/14046520411/job/39356515207

```

Assertion failed: DecodeBase64PSBT(psbt, random_string, error) == error.empty(), file D:\a\bitcoin\bitcoin\src\test\fuzz\base_encode_decode.cpp, line 95
Error processing input "D:\\a\\_temp\\qa-assets\\fuzz_corpora\\psbt_base64_decode\\0041ae3fba09a33e18a330c95acce904552b7114"