💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080421917)
Done

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080422936)
Taken but this code skipped the last byte if the input wasn't a multiple of 8, so I added that.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2080425401)
Right, I adapted this part to skip the last byte since the `BitsToBytes` function would pad again, but otherwise I think this now functions as the original fuzz test again.

💬 brunoerg commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2080425465)
Why not using `ConsumeScript` to create the scriptPubKey and scriptSig?

💬 pinheadmz commented on pull request "tests: Expand HTTP coverage to assert libevent behavior":
(https://github.com/bitcoin/bitcoin/pull/32408#discussion_r2080431221)
I noticed in the libevent's test they don't actually check that server waited at all, just that the connection closed within a few seconds of the configured time out. So I went that route, but also added a tiny minimum check just to make sure the server isn't closing immediately. And this gives us better test reliability.

So the new test is: `-rpcservertimeout=2`, we send a request, we time how long it takes to close, and that duration is expected to be between 1 and 4 seconds.

💬 keith-gardner commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2864283170)
Concept NACK

👍 ryanofsky approved a pull request: "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner"
(https://github.com/bitcoin/bitcoin/pull/32378#pullrequestreview-2826281937)
Code review ACK c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6

Thanks for the cleanup! This change should make the node interfaces.cpp more managable and organize code better. I left some suggestions for renames and API tweaks but they are not very important.

💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2080413780)
In commit "interfaces: refactor: move `waitNext` implementation to miner" (d11db291d492785125dc0971ed8eaf0a4c981932)

Would be better to return `std::unique_ptr<CBlockTemplate>` instead of `std::optional<std::unique_ptr<CBlockTemplate>>` here because unique_ptr is already nullable.

Also `node::WaitNext` is kind of a vague name. Maybe a name like `WaitAndCreateNewBlock` could be more descriptive. Or maybe just `CreateNextBlock`. Could be other ideas.

💬 ryanofsky commented on pull request "interfaces: refactor: move `Mining` and `BlockTemplate` implementation to miner":
(https://github.com/bitcoin/bitcoin/pull/32378#discussion_r2080452135)
In commit "interfaces: refactor: move `waitTipChanged` implementation to miner" (c8a3fabe4090fa2b9a0e8cef73ed5365e8221ad6)

I think name `ChainTipChanged` could be a little misleading since this returns a bool and you might think it returns true when the tip has changed, false otherwise. But in reality it returns true whether or not this tip has changed (after the timeout), and the only case it returns false is when the node is shutting down.

To be less confusing I think it would be good to
...

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2080455367)
Would using `ConsumeTransaction` make sense and just pushing that transaction into the block?

💬 kevkevinpal commented on pull request "test: added fuzz coverage for consensus/merkle.cpp":
(https://github.com/bitcoin/bitcoin/pull/32243#discussion_r2080456412)
I used `ConsumeTransaction` in [44298f0](https://github.com/bitcoin/bitcoin/pull/32243/commits/44298f0cb9c3de42b1c0f464c0d7b2779aa4c3b7)

let me know if that looks good or if you would prefer to use `ConsumeScript`

🚀 ryanofsky merged a pull request: "qa: Verify clean shutdown on startup failure"
(https://github.com/bitcoin/bitcoin/pull/30660)

👍 ryanofsky approved a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2826389039)
Code review ACK a04f17a1882407db09b0a07338e12877ac1d9e92

Thanks for digging up the history! At least it's clear how the check get here, though not exactly what it was trying to do.

💬 ryanofsky commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2080480454)
In commit "doc: warn that CheckBlock() underestimates sigops" (a04f17a1882407db09b0a07338e12877ac1d9e92)

This comment is good to have but would be nice to clarify:

1. Why it is not a problem to underestimate?
2. What this check accomplishes if isn't accurate?
3. If it might be better to remove the check or update it?

Might suggest expanding comment to something like "This check underestimates the number of sigops because it does not count witness and p2sh sigops. This is not a problem
...

🤔 w0xlt reviewed a pull request: "wallet: Keep track of the wallet's own transaction outputs in memory"
(https://github.com/bitcoin/bitcoin/pull/27286#pullrequestreview-2826415285)
reACK https://github.com/bitcoin/bitcoin/pull/27286/commits/51d4846f648e79e3e098f1f1bea7d5e823ebdea8

🤔 w0xlt reviewed a pull request: "test: refactor: negate signature-s using libsecp256k1"
(https://github.com/bitcoin/bitcoin/pull/32436#pullrequestreview-2826428063)
ACK https://github.com/bitcoin/bitcoin/pull/32436/commits/1ee698fde2e8652d8eb083749edb8029c003b8db

💬 w0xlt commented on pull request "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2864385970)
Thanks for the review and suggestion @musaHaruna . Added that text to the PR's description.

🤔 pablomartin4btc reviewed a pull request: "contrib: remove bdb exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/32448#pullrequestreview-2826445260)
cr ACK f9dfe8d5e0d3f628659702ab781b7919505c829f

FWIW: Tried to run the check on Ubuntu and it seems some lief symbols have [changed](https://github.com/lief-project/LIEF/blob/main/doc/sphinx/changelog.rst) previously to the latest released version (< 0.16.5 - eg `lief.EXE_FORMATS.ELF` => `lief.ELF.Binary`), corrected it locally and ran succesfully, then saw that the CI has 0.13.2, this works well too.

💬 murchandamus commented on pull request "coinselection: Optimize BnB exploration":
(https://github.com/bitcoin/bitcoin/pull/32150#issuecomment-2864494772)
The preceding PR https://github.com/bitcoin/bitcoin/pull/29532 got merged, so this is now ready for review.

👍 hodlinator approved a pull request: "cmake: Add application manifests when cross-compiling for Windows"
(https://github.com/bitcoin/bitcoin/pull/32396#pullrequestreview-2826464805)
ACK 477b69ad19d7fe37de9ca23926bcf007b309876d

### Concept

Paves the way for more intentional Windows manifest usage. [Linked documentation](https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests) confirms that it can be used to control codepage and specify supported Windows versions.

### Approach

Not something I could have easily written from scratch but makes logical sense in how the new CMake function `add_windows_application_manifest` generates a manifest file
...