💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#issuecomment-2864238675)
I'm reworking this target, will draft it.

📝 brunoerg converted_to_draft a pull request: "fuzz: wallet: add target for spkm migration"
(https://github.com/bitcoin/bitcoin/pull/29694)
This PR adds a fuzz target for `ScriptPubKeyMan` migration. It creates a `LegacyDataSPKM` which can have some keys/scripts/etc, and then migrate it to descriptor. I tried to keep it as compatible as possible with future legacy wallet removal.

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

💬 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.