💬 melvincarvalho commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2218357984)
Great analysis, thanks

I have seen two instances where the diff drops to 1. The first is when the mining dries up for 20 minutes, and then someone will mine a block, just to keep the chain going (normally wiz :)).

The other is when people are deliberately propagating blocks with a future timestamp, up to about 6 blocks. Coincidentally 6 * 20 = 2 hours. I do not know what triggers this.

The 20 minute rule when used properly is quite nice as it keeps the chain flowing. The future spoo
...

💬 ryanofsky commented on pull request "rpc: Use untranslated error strings in loadtxoutset":
(https://github.com/bitcoin/bitcoin/pull/30395#discussion_r1670972864)
In commit "rpc: Use untranslated error strings in loadtxoutset" (fa5b8920be041380fbfa4c7b443918637423d7a0)

Not important, but you could change this from `bilingual_str&&` to just `bilingual_str` so it is simpler, and function can be called naturally with both lvalues and rvalues, not just rvalues. Rvalues will be moved in any case.

👍 ryanofsky approved a pull request: "rpc: Use untranslated error strings in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/30395#pullrequestreview-2167052588)
Code review ACK fa5b8920be041380fbfa4c7b443918637423d7a0

💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2218389213)
@Christewart done, since it would need a release note anyways

👍 itornaza approved a pull request: "refactor: add coinbase constraints to BlockAssembler::Options"
(https://github.com/bitcoin/bitcoin/pull/30356#pullrequestreview-2167070268)
Concept ACK d89637eceab0145a64c2d1af1139ace204a3ab3c

Giving the option to assign the values of `coinbase_max_additional_weight` and `coinbase_output_max_additional_sigops` needed by the Stratum V2 protocol is essential for its integration. I understand the risks involved by having two parameters for the same thing:

> 1. The max block weight demanded by the user via -blockmaxweight
> 2. Weight units reserved for the coinbase

but it seems unavoidable at a first glance due to thei
...

💬 itornaza commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1670984170)
I take it you wanted to rename it to `coinbase_max_additional_weight` instead

💬 itornaza commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1670987152)
Again, should it be `coinbase_max_additional_weight?`?

💬 itornaza commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1670985574)
Same here, did you meant to name it `coinbase_max_additional_weight`?

💬 itornaza commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#discussion_r1670987990)
`coinbase_max_additional_weight`?

💬 mzumsande commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2218418674)
I also think that 2) would be the best approach, because 1) doesn't seem to remove the race, just makes it more unlikely: If a version message was received after `InitializeNode()` and before `SendInitialMessages()` I think the race would still be happening.

🚀 ryanofsky merged a pull request: "rpc: Use untranslated error strings in loadtxoutset"
(https://github.com/bitcoin/bitcoin/pull/30395)

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1671033583)
Fixed

💬 achow101 commented on pull request "descriptors: Be able to specify change and receiving in a single descriptor string":
(https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-2218456437)
> I'm not sure if it's the intended behavior, but passing the same `NUM` twice to `importdescriptors` does not fail and ends up with a single _change_ descriptor:

Good catch.

The BIP does not specify either way yet, but I don't think there's any reason to allow duplicates here, so I've implemented that and added a test.

💬 achow101 commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#issuecomment-2218543678)
ACK ad1e3620af88f73b869b4a2dbb74e03f5cd93517

💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2218643656)
The unit test failure in the CI was caused by the [`ConnmanTestMsg::Handshake`](https://github.com/bitcoin/bitcoin/blob/c06b3764fe38de90e13261213dca34d93ec07f18/src/test/util/net.cpp#L20) helper (used in the unit test [`outbound_slow_chain_eviction`](https://github.com/bitcoin/bitcoin/blob/c06b3764fe38de90e13261213dca34d93ec07f18/src/test/denialofservice_tests.cpp#L45)), where a version handshake is done manually by calling the relevant connman/peerman methods. Due to a missing call to `SendMess
...

🚀 ryanofsky merged a pull request: "fuzz: improve utxo_snapshot target"
(https://github.com/bitcoin/bitcoin/pull/30329)

💬 achow101 commented on pull request "test/BIP324: disconnection scenarios during v2 handshake":
(https://github.com/bitcoin/bitcoin/pull/29431#issuecomment-2218656952)
ACK c9dacd958d7c1e98b08a7083c299d981e4c11193

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1671191613)
https://delvingbitcoin.org/t/introduction-to-cluster-linearization/1032

🤔 ryanofsky reviewed a pull request: "fuzz: improve utxo_snapshot target"
(https://github.com/bitcoin/bitcoin/pull/30329#pullrequestreview-2167379330)
Post-merge code review ACK de71d4dece604907afc4fc26b7788e9c1a4cbecb

This seems like a good change in principle, but I'm not actually sure what additional test coverage it provides. The only valid snapshot that can be loaded seems to be one that is hardcoded for a chain with height 200 and no transactions except for coinbases. After the snapshot is generated there is only a single `ConsumeBool` call determining whether or not headers for the blocks are added before loading the snapshot.

So
...

💬 ryanofsky commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#discussion_r1671160670)
In commit "fuzz: improve utxo_snapshot target" (de71d4dece604907afc4fc26b7788e9c1a4cbecb)

Since fuzz test is not deterministic, and doesn't give us an easy way to know hash values here actually allow snapshot to be loaded, it might be good to write a separate regtest generating a an empty chain with height 200 and verifying these hashes.