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

💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2226318676)
done, thanks

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2226322535)
done as suggested

💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2226356487)
In 2bcdf53a58ee6cd7c70f2afd97a16fce5dcc21c6 "rpc: Support v3 raw transactions creation"

If `createrawtransaction` is going to support setting the version, then `createpsbt`, `walletcreatefundedpsbt`, `send`, and `sendall` need to as well.

💬 achow101 commented on pull request "wallet, rpc: add v3 transaction creation and wallet support":
(https://github.com/bitcoin/bitcoin/pull/32896#discussion_r2226343248)
In 40c6d135f37a1d8dd64cbea3ca30229c2e012fa2 "wallet: throw error at conflicting tx versions in pre-selected inputs"

We want to avoid querying `mapWallet` as much as possible for performance. `txo.GetWalletTx` returns a reference to the `CWalletTx` that can be used here instead of a `mapWallet` lookup.

👍 l0rinc approved a pull request: "test: check proper OP_2ROT behavior"
(https://github.com/bitcoin/bitcoin/pull/33047#pullrequestreview-3048598782)
ACK b94c6356a29b03def6337c91caabb3b8642187e8

I can confirm that the whole test suite passes when the stack erase is removed from the script interpreter code.

<details>
<summary>Details</summary>

```bash
rm -rf build && cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Debug && ninja -C build && build/bin/test_bitcoin --run_test=script_tests
```

```diff
diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 61ea7f4503..efb1af8137 100644
--- a/src/script/interpreter.
...

👍 ryanofsky approved a pull request: "util: Abort on failing CHECK_NONFATAL in debug builds"
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-3048587324)
Code review ACK fa68bda5f82887f95cc0313385c8e3a97eaa9967. Looks good! Since last review added some more test coverage and reverted some unneeded changes.

💬 ryanofsky commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2226398224)
re: https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2223179634

> restored this, and extended the workaround in the fuzz test, and wrote unit tests.

Changes look nice, but to be sure there still isn't a test that replaces the python test, explicitly passing a "trigger_internal_bug" argument to the echo RPC and ensuring that it returns an "Internal bug detected" message in response. The fuzz test handles this case, but doesn't try to trigger it, so won't fail if the CHECK_NONFATAL`
...

💬 ryanofsky commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2226365946)
In commit "util: Abort on failing CHECK_NONFATAL in debug builds" (fa68bda5f82887f95cc0313385c8e3a97eaa9967)

Could maybe make this less verbose by changing maybe_mock from std::unique_ptr to std::optional and using maybe_mock.emplace(). But maybe verbosity is the point here, seems fine regardless

💬 pinheadmz commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#discussion_r2226412738)
Why require anything at all here? What's the difference really between seeing 1 tx in the last *n* blocks and seeing none? For the regtest user for example, the PR like this wouldn't actually solve the issue.

🤔 furszy reviewed a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3048658400)
ACK c5c1960f9350d6315cadbdc95fface5f85f25806

💬 l0rinc commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#issuecomment-3109808579)
I have remeasured `-reindex-chainstate` speed by running the rebased PR (now that other optimizations were merged) until 900k blocks twice before and twice after, and the speedup is still ~4%:
<img width="479" height="296" alt="image" src="https://github.com/user-attachments/assets/19f2f7ee-8b0d-461a-90df-fdc610521f8b" />

<details>
<summary>Details</summary>

```python
COMMITS="7129c9ea8e950f50bdc56d88c57617c66c90bb8a f7d9f4510d537b280808e1e8e203c9445c8ad4df"; \
STO
...

👍 ryanofsky approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide wait methods in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-3048666902)
Code review ACK c6e2c31c55123cc97b4400bcbf3c37a39b067a22. Just rebased and tweaked documentation since last review.

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