💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2226418327)
In commit "rpc: unhide waitfor{block,newblock,blockheight}" (c6e2c31c55123cc97b4400bcbf3c37a39b067a22)

Accidentally dropped the word RPC?

💬 ryanofsky commented on pull request "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help":
(https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2226431850)
re: https://github.com/bitcoin/bitcoin/pull/30635#discussion_r2052789582

Thanks, I think I get it now. "Abort if RPC came out of warmup too early" meant "There is a bug if RPC came out of warmup too early, so abort". But it should be pretty obvious that if a check statement fails, there is a bug, so the comment seems redundant and your change dropping the comment and keeping the check makes sense.

👍 ryanofsky approved a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3048701490)
Code review ACK 1d9ba616c91c992e64cf7bef89f80a8bf3b6e737. No changes since last review, just rebased

📝 glozow opened a pull request: "test: reduce runtime of p2p_opportunistic_1p1c.py"
(https://github.com/bitcoin/bitcoin/pull/33048)
It was brought to my attention that the runtime of this test is Too Damn High. The test is slow due to the many `wait_for_getdata`s with delays (inbound peer + txid request) and the large volume of messages sent in the dos-related tests. This PR cuts the runtime by about 60% by reducing the number of messages/transactions and using `setmocktime` instead of waiting.

On my machine, master:
```
84.51s user 1.55s system 57% cpu 2:28.53 total
```
After first commit (about 1min faster):
```
2
...

💬 glozow commented on pull request "test: reduce runtime of p2p_opportunistic_1p1c.py":
(https://github.com/bitcoin/bitcoin/pull/33048#issuecomment-3109860743)
cc @achow101

💬 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