💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2226282376)
Fixed the typos. I didn't mention the `getorphantxs` fields since we haven't mentioned them in release notes before

💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2226286848)
Added the option again, a warning, and a more explicit recommendation in the release notes.

💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2226284019)
Thanks, removed that commit and leaving as is

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2226304853)
> In " init: provide thread pool to indexes " [82fa9d2](https://github.com/bitcoin/bitcoin/commit/82fa9d29653b445118fc2d03e2ced520a5d4c7dc)
>
> Define the max and then use it here and in checking the bounds

Since the maximum number of threads for indexes should depend on how many threads Core has at runtime and the number of available processors, I don't think hardcoding it here is the best approach. I agree with adding it on the error side: https://github.com/bitcoin/bitcoin/pull/26966#di
...

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2226307172)
> In "util: introduce general purpose thread pool" [7d984c3](https://github.com/bitcoin/bitcoin/commit/7d984c3b2dca085ef7f49d21568b06c7b89c7807)
>
> Also verify that no work queue size is 0.

sure. Done.

💬 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#issuecomment-3109644770)
concept ACK -- I think like https://github.com/bitcoin/bitcoin/pull/32528 a little fudge factor against what is "most correct" is worth the tradeoff for a more predictable user experience, especially on test networks

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

🤔 glozow reviewed a pull request: "package validation: relax the package-not-child-with-unconfirmed-parents rule"
(https://github.com/bitcoin/bitcoin/pull/31385#pullrequestreview-3048518467)
Thanks @ishaanam! Took all suggestions

💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2226318223)
changed them to `parent_relative, child_relative`

💬 glozow commented on pull request "package validation: relax the package-not-child-with-unconfirmed-parents rule":
(https://github.com/bitcoin/bitcoin/pull/31385#discussion_r2226317096)
Good point, added a definition

💬 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