💬 theStack commented on pull request "crypto, refactor: add new KeyPair class":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1705843010)
nit, if you have to retouch:
```suggestion
CKey key = GenerateRandomKey();
```

💬 mzumsande commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2271875972)
> This shouldn't send transactions without sending an INV. Instead, it should INV and wait for the GETDATA. (thanks to mzumsande for making a note of the behavior).

For context, an old discussion on this: https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1186579724 (I want to think a bit more on the potential benefits of #30572 before voicing an opinion).

👍 ryanofsky approved a pull request: "log: Remove NOLINT(bitcoin-unterminated-logprintf)"
(https://github.com/bitcoin/bitcoin/pull/30485#pullrequestreview-2221986325)
Code review ACK fa18fc705084f3620be566d8c6639b29117ccf68

💬 sipa commented on issue "Unexpected behaviour when using `sortedmulti_a` descriptor":
(https://github.com/bitcoin/bitcoin/issues/30518#issuecomment-2271899519)
@darosior I vaguely recall discussing this before, but I don't remember the reasoning. Imagine some particular application uses a specific fancy timelock/hashlock/2-of-3 combination that (when the script path is used) readily gives away it's a transaction by that particular application due to its unusual script. If the descriptor for that uses `multi_a(mobile_xpub,desktop_xpub,server_xpub)`, then the location of the key signed with will additionally publish which parties are involved on chain. U
...

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040)
Rebased due to the conflicts. More feedback has been addressed.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706004353)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706005633)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.

💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1706006163)
Addressed in https://github.com/bitcoin/bitcoin/pull/30454#issuecomment-2271981040.

🚀 ryanofsky merged a pull request: "log: Remove NOLINT(bitcoin-unterminated-logprintf)"
(https://github.com/bitcoin/bitcoin/pull/30485)

👍 murchandamus approved a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2222087875)
tACK 6bfa26048dbafb91e9ca63ea8d3960271e798098

Reviewed code changes and resynced the node from scratch

💬 murchandamus commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706016674)
Are we sure that we will drop support next release? Otherwise it might be better to be a bit less specific:

```suggestion
argsman.AddArg("-testnet", "Use the testnet3 chain. Equivalent to -chain=test. Support for testnet3 is deprecated and will be removed in a future release. Consider moving to testnet4 now by using -testnet4.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
```

💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1706054721)
done with the latest push.

💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2272057994)
> It may be worth logging something every time we ignore a peer that is on another chain.

[e977c69 ](https://github.com/bitcoin/bitcoin/commit/e977c698f97ac9bba30e4e3837f41721841c28c4)to [49d569c](https://github.com/bitcoin/bitcoin/commit/49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb):
Added a log message and slightly reworded a comment.

💬 LarryRuane commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-2272060737)
Force pushed rebase to fix conflict and also did some additional testing, I added several fake logging categories to ensure there can be more than 32 categories (the current limit), which is the main benefit of this pull. Also, I re-ran the benchmark tests (including with more than 32 categories), and this PR has no measurable performance impact.

@vasild and @ryanofsky, maybe you can take a look since you know this area, thanks.

💬 fjahr commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#issuecomment-2272064338)
re-ACK 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb

Only trivial changes addressing comments, per diff: https://github.com/bitcoin/bitcoin/compare/e977c698f97ac9bba30e4e3837f41721841c28c4..49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb

💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1706068877)
I think it's better to be precise and we could still shift it if that becomes absolutely necessary for a good reason. It seems better to have people prepare for it to be dropped in the next release unless there is good reason brought forward for shifting. This is better than the alternative that we are vague and then people claim last minute we can not do it in the next release already because we were not clear enough. I don't want to give an invite to the "Testnet3 is the real testnet" crowd :)
...

👍 ryanofsky approved a pull request: "test: [refactor] Use g_rand_ctx directly"
(https://github.com/bitcoin/bitcoin/pull/30571#pullrequestreview-2222184242)
Code review ACK fa295215f6fa3b85b0387511920f75eeb3e12b58 but I think it would be nicer to add `FastRandomContext& m_rng{g_rand_ctx}` to `BasicTestingSetup` and have most test code use `m_rng` instead of `g_rand_ctx` directly.

I think this would be nicer mostly as a matter of style since most test code now doesn't reference global variables directly, while this PR is adding hundreds of references. But it could also help down the road, for example if we wanted to remove the global, or run test
...

💬 naiyoma commented on pull request "test: Use Decimal for fee calculations in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1706078135)
I was aiming for exact precision because of this comment https://github.com/bitcoin/bitcoin/pull/23225#issuecomment-938801112, but I get your point. Maybe if fee estimation changes to decimals internally in future, the tests can be modified accordingly.

💬 naiyoma commented on pull request "test: Use Decimal for fee calculations in feature_fee_estimation.py":
(https://github.com/bitcoin/bitcoin/pull/29566#discussion_r1706083059)
@maflcko IIUC reverting to the earlier version would mean that I also need to discard all the changes made to the functions `check_raw_estimates` and `check_smart_estimates`, right?

💬 petertodd commented on pull request "[27.x] Even more backports":
(https://github.com/bitcoin/bitcoin/pull/30558#discussion_r1706083475)
Worth crediting @1440000bytes too here, as they finalized my pull-req.