💬 l0rinc 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-3109894500)
While I don't think this is a measurable optimization (i.e. RPCs won't get faster because of it), it does make sense to reduce useless work and memory fragmentation.
We're reserving the right amounts - and can do other such cases (e.g. https://github.com/bitcoin/bitcoin/blob/master/src/rpc/rawtransaction.cpp#L1246) in follow-ups.

```bash
rm -rfd build && cmake -B build -DBUILD_BENCH=ON -DCMAKE_BUILD_TYPE=Release && cmake --build build -j$(nproc) && build/bin/bench_bitcoin -filter='BlockToJs
...

👍 ryanofsky approved a pull request: "refactor,test: follow-ups to multi-byte block obfuscation"
(https://github.com/bitcoin/bitcoin/pull/33039#pullrequestreview-3048768967)
Code review ACK e7b00d69b6e05b6b4fd08ace785727c56626cbc2. Thanks for the followups!

💬 darosior commented on pull request "script: return verification flag responsible for error upon validation failure":
(https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3109930939)
That doesn't seem too unreasonable to stop disconnecting on receiving a transaction which fails Script validation due to a consensus verification flag. However it's not clear that it's necessarily better. It doesn't help in adversarial situations, but may still avoid wasting resources on innocently misbehaving peers. With this PR, opportunistically detecting such peers and evicting them comes at virtually no cost (unlike the current situation). So i suppose the tradeoff is between "maintain the
...

💬 ryanofsky commented on pull request "refactor,test: follow-ups to multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/33039#discussion_r2226491020)
In commit "refactor: standardize obfuscation memory alignment" (e7b00d69b6e05b6b4fd08ace785727c56626cbc2)

Looks good to declare an obfuscation object and let it serialize itself, and not have an unnecessary read call. I still think the "making sure we don't obfuscate the key" comment is vague, because it's not clear what it refers to. I'd suggest moving that comment next to the assert and moving the assert where it's more relevant:

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

```diff
--- a/sr
...

💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#issuecomment-3109936379)
Rebased as well

💬 l0rinc commented on pull request "refactor,test: follow-ups to multi-byte block obfuscation":
(https://github.com/bitcoin/bitcoin/pull/33039#discussion_r2226519739)
Done, thanks

🤔 furszy reviewed a pull request: "p2p: Advance pindexLastCommonBlock early after connecting blocks"
(https://github.com/bitcoin/bitcoin/pull/32180#pullrequestreview-3048831581)
ACK 3bdff9a154733f8f9f379448f5595a2e90474bc6

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