💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2152685201)
> Concept ACK. Mind sharing how you see this fitting in with Erlay work?
>
> Will review in more depth soon.

Sure. For Erlay purposes, some transactions need to be sent via fanout even to Erlay-enabled peers. In order to know to how many peers to send this via fanout, we need to count the amount of tx-relaying peers that we have, both inbound and outbound. This is done, in the current patch, by going over all peers when constructing their corresponding INV message and deciding whether to a
...

💬 ryanofsky commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1629646400)
In commit "validation: Make ReplayBlocks interruptible" (26e78f2ee4a325f20c3b3bfaac5be532ab9e7bac)

> I switched to the current approach of detecting this implicitly inside `BatchWrite()` because it seemed more elegant and led to less boilerplate code, but it's still an option.

I would definitely be curious to see the other approach. Not just because it might be better at detecting inconsistent states like Luke is suggesting, but also because it seems like it would make the code more clear.
...

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2152711889)
i was a bit tired of seeing "you need to rebase" messages, will get around to this 😄

💬 maflcko commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1629711039)
If the return value is wrong or unused, it could make sense to remove it? Also, it could make sense to update the pattern `if (peer && ...` in the other cases as well for consistency, if this is correct.

💬 maflcko commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2152770109)
ACK 15796d4b61342f75548b20a18c670ed21d102ba8

💬 maflcko commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#discussion_r1629726335)
I agree, but for consistency with the other `def ser_compact_size` and the other code, I'll leave as-is for now.

👍 dergoegge approved a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237#pullrequestreview-2102290054)
Code review ACK 4c99301220ab44e98d0d0e1cc8d774d96a25b7aa

I've verified that this can reproduce the nullptr deref bug by reverting a8203e94123b6ea6e4f4a6320e3ad20457f44a28 and making short id collisions more likely (see top commits on: https://github.com/dergoegge/bitcoin/commits/2024-06-cmpctblk-extra-txn-test/).
```
$ n=0; while (( n++ < 100 )); do src/test/test_bitcoin --run_test=blockencodings_tests/ReceiveWithExtraTransactions ; done
...
Running 1 test case...
unknown location(0): fa
...

👍 hebasto approved a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2102299643)
re-ACK e4ecb8217ada3dae1c1645a8d0a12e14b0f935da, I agree with changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2088736908).

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1629750737)
This copy assignment can be amended to handle the self-assignment as well to do not conflict with https://github.com/bitcoin/bitcoin/pull/30234.

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1629747141)
Does it make sense to label branches with the `[[likely]]` and `[[unlikely]]` attributes here?

💬 murchandamus commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583)
The file name could also be renamed to refer to TRUC transactions instead of v3_policy.

🤔 murchandamus reviewed a pull request: "policy: bump TX_MAX_STANDARD_VERSION to 3"
(https://github.com/bitcoin/bitcoin/pull/29496#pullrequestreview-2102303518)
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1629758949)
Yes, interesting edge case! I extended the comment.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1629759103)
Done

📝 pinheadmz opened a pull request: "json-rpc 2.0 followups: docs, tests, cli"
(https://github.com/bitcoin/bitcoin/pull/30238)
This is a follow-up to #27101.

- Addresses [post-merge comments ](https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1606723428)
- bitcoin-cli uses JSON-RPC 2.0
- functional tests use JSON-RPC 2.0 by default (exceptions are in the regression tests added by #27101)

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629801393)
I think the meaning is well understood, both before and after `LegacyScriptPubKeyMan` is deleted. `SPKM` is an abbreviation for `ScriptPubKeyMan` anyways.

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629814874)
The line this comment is on is in the one that goes over the non-HD keys. However, I see that you're confused about the similar line in the loop below that iterates the hd chains.

The `keypool_size` here is a runtime value that depends on the startup options. It's is set when the `DescriptorScriptPubKeyman` is instantiated on loading, and the value is never persisted to disk.

The value that does matter is the `range_end` stored in `WalletDescriptor`. This is persisted to disk. When we are
...

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629821488)
Added a comment.

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1629823159)
I don't think it's necessary to have a comment saying such.

💬 maflcko commented on pull request "json-rpc 2.0 followups: docs, tests, cli":
(https://github.com/bitcoin/bitcoin/pull/30238#discussion_r1629850553)
I'd say absolute links make sense in this case, because the file will be moved without modification later on into a different folder (`./doc/release-notes/`)