💬 furszy commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#discussion_r2226532525)
nano non-blocking nits:
Could extract `tip = ActiveTip()` and use it everywhere here to make it slightly more readable.

Also, this isn’t introduced by this PR, but the second check has always puzzled me a bit. It would be nice to have some util function like `IsBlockInChain(const CBlockIndex* block_to_test, const CBlockIndex* chain_tip)`.

💬 achow101 commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-3109997312)
ACK 5d82d92aff7c11ce17ee809c060e37f73a8a687a

👍 ryanofsky approved a pull request: "refactor,test: follow-ups to multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/33039#pullrequestreview-3048910436)
Code review ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0, just tweaking key write assert as suggested

💬 ryanofsky commented on pull request "refactor,test: follow-ups to multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/33039#discussion_r2226584013)
In commit "test: make `obfuscation_serialize` more thorough" (2dea0454254180d79464dc6afd3312b1caf369a7)

Could check this more strictly with:

<details><summary>diff</summary>
<p>

```diff
--- a/src/test/streams_tests.cpp
+++ b/src/test/streams_tests.cpp
@@ -65,17 +65,19 @@ BOOST_AUTO_TEST_CASE(obfuscation_serialize)
std::vector key_in{m_rng.randbytes<std::byte>(Obfuscation::KEY_SIZE)};
DataStream ds_in;
ds_in << key_in;
- BOOST_CHECK_EQUAL(ds_in.size(), 1 + Obfus
...

🚀 achow101 merged a pull request: "RPC: Add reserve member function to `UniValue` and use it in `blockToJSON` function"
(https://github.com/bitcoin/bitcoin/pull/31179)

💬 darosior commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3110064945)
One example where the opportunistic disconnection could be useful is in the case of a hardfork coin with signature replay protection but still connecting to Bitcoin peers. It happened in the past but admittedly seems very unlikely nowadays.

🤔 Prabhat1308 reviewed a pull request: "wallet: Set minversion to FEATURE_LATEST during migration"
(https://github.com/bitcoin/bitcoin/pull/33041#pullrequestreview-3048953524)
Agree with the comment by @pablomartin4btc here that if we want to go the route of keeping the wallet Version , its better to set it as a constant since `FEATURE_LATEST` is the only version being maintained in the practical sense and rename the function along the lines of `SetVersion` . Then we can even go ahead with the modified https://github.com/bitcoin/bitcoin/pull/32977 cleanup .

💬 l0rinc commented on pull request "refactor,test: follow-ups to multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/33039#discussion_r2226638379)
Thanks, I'll consider it next time I'm pushing

🤔 w0xlt reviewed a pull request: "wallet: Set minversion to FEATURE_LATEST during migration"
(https://github.com/bitcoin/bitcoin/pull/33041#pullrequestreview-3049035879)
ACK https://github.com/bitcoin/bitcoin/pull/33041/commits/5aa758861cf1399842fd0bea3a42d5b0cafcb0f6

💬 achow101 commented on pull request "test: reduce runtime of p2p_opportunistic_1p1c.py":
(https://github.com/bitcoin/bitcoin/pull/33048#issuecomment-3110151117)
The runtime is now much better, thanks!

ACK eb65f57f319dc4e2ea8c83cf7e283c36f1c0d53b

I checked that the changes looked sane, but did not exhaustively walkthrough the scenarios to determine whether evictions were occurring.

💬 w0xlt commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2226679396)
Is there any specific reason not to use `unsigned char` directly, like `std::span<const unsigned char > sp)` ?

💬 achow101 commented on pull request "descriptors: MuSig2":
(https://github.com/bitcoin/bitcoin/pull/31244#discussion_r2226689907)
Will change if I need to retouch.

💬 furszy commented on pull request "index: fix wrong assert of current_tip == m_best_block_index":
(https://github.com/bitcoin/bitcoin/pull/32878#issuecomment-3110184342)
@HowHsu, are you going to include the test or a patch to reproduce the issue? I’m happy to ACK it either way.

💬 w0xlt commented on pull request "wallet: Store transactions in a separate sqlite table":
(https://github.com/bitcoin/bitcoin/pull/33034#issuecomment-3110194893)
Concept ACK

💬 achow101 commented on pull request "wallet, sqlite: Encapsulate SQLite statements in a RAII class":
(https://github.com/bitcoin/bitcoin/pull/33033#discussion_r2226706401)
In theory we could change `uint256`'s `value_type`, and we wouldn't need to make any changes here in that case.

🤔 w0xlt reviewed a pull request: "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt"
(https://github.com/bitcoin/bitcoin/pull/32895#pullrequestreview-3049112073)
ACK https://github.com/bitcoin/bitcoin/pull/32895/commits/b0470f573735a04ebc87ca5d3e00a73e3f10fee1

nit: Commit https://github.com/bitcoin/bitcoin/pull/32895/commits/3662960470d74850008b7b48b6820d02002f7a5d appears to have CI failures.

🤔 w0xlt reviewed a pull request: "wallet: Set migrated wallet name only on success"
(https://github.com/bitcoin/bitcoin/pull/32984#pullrequestreview-3049114357)
reACK https://github.com/bitcoin/bitcoin/pull/32984/commits/060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542

💬 achow101 commented on pull request "wallet: Prepare for future upgrades by recording versions of last client to open and decrypt":
(https://github.com/bitcoin/bitcoin/pull/32895#issuecomment-3110226273)
> nit: Commit [3662960](https://github.com/bitcoin/bitcoin/commit/3662960470d74850008b7b48b6820d02002f7a5d) appears to have CI failures.

I believe that's only because the tasks were canceled by a new push.

💬 Karim8090 commented on issue "guix: Zip file non-determinism when building in WSL":
(https://github.com/bitcoin/bitcoin/issues/32931#issuecomment-3110253591)
Babu8090

🤔 w0xlt reviewed a pull request: "wallet: Set descriptor cache upgraded flag for migrated wallets"
(https://github.com/bitcoin/bitcoin/pull/33031#pullrequestreview-3049159589)
ACK https://github.com/bitcoin/bitcoin/pull/33031/commits/4e5faaba82b4e1f42edb3a52270ea9086c1edfa9