💬 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/`)

📝 instagibbs opened a pull request: "Ephemeral Anchors, take 2"
(https://github.com/bitcoin/bitcoin/pull/30239)
A replacement for https://github.com/bitcoin/bitcoin/pull/29001

Now that we have 1P1C relay, TRUC transactions and sibling eviction, it makes sense to retarget this feature more narrowly by not introducing a new output type for now, and simple focusing on the feature of allowing temporary dust in the mempool.

Users of this can immediately use dust outputs as:
1. Single keyed anchor
2. Single unkeyed anchor, ala P2WSH(OP_TRUE), P2SH(OP_TRUE)

Future upgrades can include:
1. Adding `OP_
...

✅ instagibbs closed a pull request: "Ephemeral Anchors"
(https://github.com/bitcoin/bitcoin/pull/29001)

💬 instagibbs commented on pull request "Ephemeral Anchors":
(https://github.com/bitcoin/bitcoin/pull/29001#issuecomment-2153063105)
closing this in favor of https://github.com/bitcoin/bitcoin/pull/30239

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1629970936)
Done.