💬 achow101 commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#issuecomment-2155151279)
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c

💬 achow101 commented on pull request "wallet: Migrate legacy wallets to descriptor wallets without requiring BDB":
(https://github.com/bitcoin/bitcoin/pull/26596#discussion_r1631443918)
It is theoretically possible to have a descriptor wallet using BDB under 2 scenarios, however this is still an unsupported configuration:

1. The descriptor wallet was created on an unreleased version after #16528 but before #19077 (there was a period of about 6 months where this could have happened)
2. The user manipulated the database using external tooling where they created a BDB file and manually inserted descriptor wallet records

🚀 achow101 merged a pull request: "policy: bump TX_MAX_STANDARD_VERSION to 3"
(https://github.com/bitcoin/bitcoin/pull/29496)

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631387475)
nit: the commit message says both wipes should be possible independent of each other, so I think coupling them here is not great (i'm also open to adding a `do_reindex` variable to avoid duplication).
```suggestion
options.wipe_chainstate_db = m_args.GetBoolArg("-reindex", false) || m_args.GetBoolArg("-reindex-chainstate", false);
```

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631415537)
nit
```suggestion
<< "\t" << "Blockfiles Indexed: " << std::boolalpha << chainman.m_blockman.m_blockfiles_indexed.load() << std::noboolalpha << std::endl
```

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631413302)
nit: could be interpreted as "any blockfiles have been added", so perhaps this is less ambiguous:
```suggestion
* Whether all blockfiles have been added to the block tree database. Normally
```

🤔 stickies-v reviewed a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2104869562)
Code LGTM 682f1f1827dff78208bc1272f82d217c42a2cd69 modulo one small bug in comments.

I couldn't find or think of any occasions where the new reduced wiping behaviour introduces problematic behaviour. The new variable names introduced make things significantly more clear and is a very welcome change.

💬 stickies-v commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1631416825)
I think this has to be the inverse, but just "reverting back" to `!do_reindex` is probably more clear:
```suggestion
if (!do_reindex && !do_reindex_chainstate && chain_active_height <= 1) {
```

💬 achow101 commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-2155189032)
Rebased

> Seems like this is a remaining mention of `CTransaction::nVersion`?

Good catch, fixed.

👍 tdb3 approved a pull request: "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16"
(https://github.com/bitcoin/bitcoin/pull/28312#pullrequestreview-2105015222)
re ACK for 5cf0a1f230389ef37e0ff65de5fc98394f32f60c
Re-ran functional tests (passed) and induced a failure by hard coding 1 as the k value.

💬 tdb3 commented on pull request "test: fix `keys_to_multisig_script` (P2MS) helper for n/k > 16":
(https://github.com/bitcoin/bitcoin/pull/28312#discussion_r1631478208)
nit: In case this file is touched, it might be clearer (as rkrux mentioned) to add a comment explicitly explaining the math, although this is just a nice-to-have since the addition breaks it out such that the reader is provided a hint (typical multisig script).
Author's preference.

```diff
diff --git a/test/functional/test_framework/script_util.py b/test/functional/test_framework/script_util.py
index 855f3b8cf5..69be490a67 100755
--- a/test/functional/test_framework/script_util.py
+++ b/
...

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631480159)
I haven't benchmarked or investigated compiled code, but I want to give the compiler every opportunity to use the simpler operation here (the conditional `Set` is more complex, and inlining will hopefully strength-reduce it, but I prefer not counting on that).

👍 tdb3 approved a pull request: "json-rpc 2.0 followups: docs, tests, cli"
(https://github.com/bitcoin/bitcoin/pull/30238#pullrequestreview-2105025972)
ACK for 1f6ab1215bbb1f8a5f1743c3c413b95ad08090df
Great work and thanks for following up with the comments.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631489780)
Done.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631489851)
Done.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631490140)
Added coverage for the `swap` function in the fuzz test.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631490229)
Done.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631490300)
Done.

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631490491)
Same reasoning as [here](https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631480159)

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631490602)
Done.