👍 dergoegge approved a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2104888365)
tACK 030c9edf5b12033207da2bc0735f97840dc88056

I differentially fuzzed the muhash implementation we have on master against the version in this PR. Given the coverage reached and assuming that the implementation on master is correct, I am confident that the muhash implementation in this PR is also correct. I tested across multiple compilers and optimization levels i.e. the following tuples: `(master clang -O2, pr clang -O2)`, `(master clang -O2, pr gcc -O2)`, `(master clang-O2, pr gcc -O0)` (al
...

💬 iw4p commented on pull request "test: switch from curl to urllib for HTTP requests":
(https://github.com/bitcoin/bitcoin/pull/29970#issuecomment-2155110688)
> One drawback of this change is that a user doesn't see download progress per file any more (relevant if e.g. servers are slow or the connection is bad for any other reason; for me this currently takes several minutes per file :/) and hence is left in the dark whether there is still activity, so Concept ~0.

I appreciate your feedback. While it's true that switching from `curl` to `urllib` removes the per-file download progress visibility, this change offers significant improvements in code c
...

🤔 instagibbs requested changes to a pull request: "util: add BitSet"
(https://github.com/bitcoin/bitcoin/pull/30160#pullrequestreview-2104783238)
c99063e902c810dde1742696b3140610120d391c seems to need `swap` fuzz coverage

rest of comments are nits

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631369553)
```Suggestion
/* First / Last */
```

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631335816)
```Suggestion
/** Assign from a list of positional values. */
```

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631393064)
lacks coverage?

my local coverage generation is kinda screwed up, but fuzz coverage should be computed :pray:

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631406724)
maybe just use conditional `Set`?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631403529)
Set(i)

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631406944)
maybe just use conditional `Set`?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631408015)
stick with `pos` for arg names?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631403413)
Set(i)?

💬 instagibbs commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1631411035)
just check `!None()`?

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1631431050)
Thanks for pointing this out. The background validation does not seem to finish in this case.

I am now testing with a new node `n3` (in my local copy) to avoid the manual rollback to `START_HEIGHT` ([L207](https://github.com/bitcoin/bitcoin/pull/29996/files#diff-8eaa6b77b4e4f242deb67950425cb6b6b7de5370c56a1e8fa7e3a12e95df0a9dR207)). Additionally, `sync_blocks()` was throwing a timeout when reusing n2 for this test (not sure why).

Here's the new approach I'm trying:

- The new node start
...

💬 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.