💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382674470)
In 1181568816e32f378f3f9a50a0658d96541771de "sign: Create MuSig2 signatures for known MuSig2 aggregate keys" (but also in the 3 previous commits)

The original impetus of this suggestion was for the reader to avoid reading redundant code, but having the `sighash` calculated only once during MuSig2 signing flow for a `script_pubkey` seems better from performance POV as well, even though this redundant calculation is inside wallet and not in the more time sensitive operations of the node. This a
...

💬 rkrux commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2382697115)
In 02994c2cbe2f051b868f49e65fac042feace2edf "test: Test MuSig2 in the wallet"

In continuation to the previous suggestion of handling the cases where the `musig` descriptor is not known to the wallet and instead only few keys are that could sign - https://github.com/bitcoin/bitcoin/pull/29675/#discussion_r2368738533 (thanks for adding the fix btw).

When I was debugging this case earlier, 2 tests failed because of incorrect number of nonces added - earlier `send` RPC was used instead of `wal
...

💬 HowHsu commented on pull request "index: handle case where pindex_prev equals chain tip in NextSyncBlock()":
(https://github.com/bitcoin/bitcoin/pull/32875#discussion_r2382744473)
> Not sure what that means, we write tests to tell us when the behavior of the application changed. You have changed the behavior and no test broke, so there's a lack of test coverage that we should fix to explain why this change is

this PR doesn't change the behavior of `NextSyncBlock()`, it just tweaks the code to make the function clearer.

💬 zaidmstrr commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#issuecomment-3339174566)
Rebased. And addressed the changes done in [PR #33230](https://github.com/bitcoin/bitcoin/pull/33230).

💬 adam3us commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#discussion_r2382752997)
The warning "deprecated & discouraged" doesn't sound consistent with @bitschmidty's use case for miners wanting to set this parameter

> Some miners want to use Bitcoin Core for block template building but dont want to include large op_returns.

https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3320079835

I didn't see any commentary disagreeing with it being reasonable for miners to want that, and in fact miners are the node type uniquely in a position to actually curb large dat
...

💬 adam3us commented on pull request "docs: Undeprecate datacarrier and datacarriersize configuration options":
(https://github.com/bitcoin/bitcoin/pull/33453#issuecomment-3339197218)
> @ajtowns wrote:
>
> > For comparison, the difficulty increase from early May to now (given the same hashrate) implies a decrease in revenue of 19.5%, more than 100x as much. I think it makes sense to set defaults to maximise even small amounts of revenue, but that's a "picking up pennies" saving, not a "small miners will be put out of business" situation.
>
> Is the argument that poor compact block reconstruction rates leads to miner centralization overblown then if the loss in revenue d
...

🚀 glozow merged a pull request: "key: use static context for libsecp256k1 calls where applicable"
(https://github.com/bitcoin/bitcoin/pull/33399)

💬 maflcko commented on issue "use -loadblock to load blk*****.dat files, but the blocks in it are not recognized":
(https://github.com/bitcoin/bitcoin/issues/33280#issuecomment-3339296357)
> I don't know if that was discussed when block obfuscation was added (FYI [@maflcko](https://github.com/maflcko)).

I wasn't aware of a use-case to load raw block files without first linearizing them. As suggested, the workaround here would be `-blocksxor=0` for the node that generates block files to import. You could also hack together a trivial python script to do the xor for you, quickly. (The linearize script may also work, if you omit the unneeded block hashes, but I haven't tried this).

🚀 glozow merged a pull request: "test/refactor: use test deque to avoid quadratic iteration"
(https://github.com/bitcoin/bitcoin/pull/33313)

💬 0xB10C commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3339341600)
Approach ACK.

Nice. I had noticed that we announce INVs to everyone at the same time, but it didn't occur to me that this is a fingerprinting leak. To test this change, I got creative and compared a 30.0 release candidate node with a node running this PR using my [p2p-circle](https://github.com/0xB10C/peer-observer/blob/master/tools/websocket/www/p2p-circle.html) peer-observer tool.

In the middle is our node and around it are our peers. Here, colored by the network which we are connected t
...

💬 RandyMcMillan commented on pull request "rpcconsole: display signet challenge":
(https://github.com/bitcoin-core/gui/pull/896#issuecomment-3339414175)
use HexStr() per [pablomartin4btc](https://github.com/pablomartin4btc) suggestion

📝 purpleKarrot opened a pull request: "CMake: Add dynamic test discovery"
(https://github.com/bitcoin/bitcoin/pull/33483)
Instead of parsing the test names from the source code at configure time, query the list of tests from the test executables at testing time.

This should be upstreamed to CMake: https://gitlab.kitware.com/cmake/cmake/-/issues/26920

Left to do: The `test_bitcoin-qt` executable should be fixed to support the `-function` option documented here: https://doc.qt.io/qt-6/qtest-overview.html#testing-options so that its test cases can be discovered as well.

💬 purpleKarrot commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3339779236)
Please see https://github.com/bitcoin/bitcoin/pull/33483 for dynamic test discovery.

📝 theStack opened a pull request: "doc: rpc: fix case typo in `finalizepsbt` help (final_scriptwitness)"
(https://github.com/bitcoin/bitcoin/pull/33484)
The lower-case spelling matches the `decodepsbt` result field:
https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/src/rpc/rawtransaction.cpp#L871
https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/src/rpc/rawtransaction.cpp#L1253

👍 l0rinc approved a pull request: "doc: rpc: fix case typo in `finalizepsbt` help (final_scriptwitness)"
(https://github.com/bitcoin/bitcoin/pull/33484#pullrequestreview-3273312375)
ACK ff05bebcc4262966b117082a67dc4c63a3f67d2d

The doc seems to be the only mention of `final_scriptWitness`, every other one is indeed `final_scriptwitness` - seems it was added incorrectly in https://github.com/bitcoin/bitcoin/commit/c27fe419efb3b6588c400d764122ffb33375e028#diff-a58e7bb9d9a8a0287c0b7281d99da4e79b6f8c2a5780c24c6d76c14212c48640R1624

💬 l0rinc commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2383126356)
Not necessarily, but it bothers me that we (or at least I) don't understand what's causing the problem in the first place, the current solution feels like a work-around to me - which is still better than what we had, but it's also arguably more complicated and it doesn't seem to fix the underlying problem of why reindex is missing headers in the first place. And are we sure it's a reindex-only bug or can it happen for reindex-chainstate or IBD as well? I haven't dug too deep into this, but I wan
...

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383164921)
The test failure with `send` was because of sighash types, not that `send` doesn't work for musig. It was failing on the test that changes the sighash type.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2383168777)
The sighash is calculated multiple times during signing outside of musig signing too. I will leave refactoring of this to a followup.

💬 mzumsande commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2383171731)
> Not necessarily, but it bothers me that we (or at least I) don't understand what's causing the problem in the first place

I tried to describe this in the linked issue, I think the problem is understood. To rephrase it:

Normal IBD has the order 1) Download entire header chain until we have crossed minchainwork 2) Start downloading blocks
=> assumevalid will be applied because we have a header with minchainwork and the assumevalid block in our index

During reindex, we 1) Delete all ex
...

💬 maflcko commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3340123470)
Seems nice, but in the great picture there is still room for improvement:

* This doesn't address the issue of running cross builds on the target (https://github.com/bitcoin/bitcoin/blob/200150beba6601237036bc01ee10f6a0a2246c3d/.github/workflows/ci.yml#L388)
* The functional tests aren't integrated, so they are run sequentially after the unit tests

I am not going to block this pull, because it is a clear improvement over master, and the other issues should be fixed in separate pulls. Howev
...