💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2223796103)
Yeah, I think it's a good idea to add an error saying "this option doesn't do anything anymore, remove it from your bitcoin.conf" and continue.

Do you think it's worthwhile to still support `-maxorphantxs=0` e.g. for memory-conscious users?

💬 glozow commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2223800920)
Hm, I didn't think so either, but wonder what could have caused https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2217735632 🤔

💬 sipa commented on pull request "p2p: TxOrphanage revamp cleanups":
(https://github.com/bitcoin/bitcoin/pull/32941#discussion_r2223809881)
Hmm, my best guess is a compiler/sanitizer bug.

Try something like

```c++
auto rng_seed = rng.rand256();
FastRandomContext rand_ctx(rng_seed);
```

?

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2223829051)
> Hmm I can imagine that this can be blocking when you want to stop instantly; but I guess there is a reason why you did not return here and then empty the work queue in Stop.

Yes. We need to fulfill all promises so there are no dangling futures waiting for the worker to finish executing the task.
In the future, we could avoid this by tracking all the promises and triggering a "shutdown" exception during stop.

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2223838649)
> Why 0?

Parallel sync is disabled by default. We’re currently not tracking the number of threads spawned by Core, so I chose not to make assumptions here (don't want indexes threads competing with net/validation ones). It’s safer to let users specify the appropriate number for their setup. In the future, we could improve this by adding thread tracking object/mechanism and picking up the best number for their machine.

👍 pablomartin4btc approved a pull request: "test: Do not pass tests on unhandled exceptions"
(https://github.com/bitcoin/bitcoin/pull/33001#pullrequestreview-3044920696)
tACK faa3e684118bffa7a98cf76eeeb59243219df900

Managed to reproduce the issue with the patch provided in the PR description, this branch fixes it. Nice detail handling `KeyboardInterrupt` for debugging purpose. Code-wise refactoring: better reading removing redundant catches.

👍 stickies-v approved a pull request: "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs"
(https://github.com/bitcoin/bitcoin/pull/32845#pullrequestreview-3044915254)
re-ACK af0ad72a0ed778c7e6f83b1a290705cefe23d78a

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223852456)
nit: what's the purpose of wrapping this in `namespace wallet`?

Relatedly, should probably make `TestWalletName` `static`.

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223857169)
nit: can keep comma to avoid touching this line

💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2223866061)
```suggestion
- `unloadwallet` - Return RPC_INVALID_PARAMETER when both the RPC wallet endpoint
and wallet_name parameters are unspecified. Previously the RPC failed with a JSON
parsing error.
```

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

Makes sense, I think we can make the bounds not just an arbitrary number but tied it to the cores of the machine.

That will prevent footgoon whereby user will spawn more threads than the machine can handle leading to degraded performance due to lots of context switching.

✅ achow101 closed a pull request: "test: functional test for incomplete PSBT with additional signatures required"
(https://github.com/bitcoin/bitcoin/pull/33035)

💬 achow101 commented on pull request "test: functional test for incomplete PSBT with additional signatures required":
(https://github.com/bitcoin/bitcoin/pull/33035#issuecomment-3104989559)
What is the purpose of this test? There are already multiple tests that do exactly what you have added, there is nothing useful being added here.

Please do not use a LLM to write your code.

💬 achow101 commented on pull request "Enable `-natpmp` by default":
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3105012258)
Concept ACK

💬 achow101 commented on pull request "test: delete commented-out tests and add a test case in wallet_signer":
(https://github.com/bitcoin/bitcoin/pull/33020#issuecomment-3105022393)
ACK da318fe53fa954cc3eadd7c39e17eb3da7c3e09e

💬 achow101 commented on pull request "rpc: Fix internal bug in descriptorprocesspsbt when encountering invalid signatures":
(https://github.com/bitcoin/bitcoin/pull/33014#issuecomment-3105042497)
The `CHECK_NONFATAL` is correct. `complete == true` means that the PSBT is complete and can be finalized. The invariant here is that `complete == true` means `FinalizeAndExtract` must succeed.

BIP 174 states that a finalizer must only construct valid scriptSigs and witnesses. We encounter this error because we assume that if a PSBT is finalized, the scriptSigs and witnesses are valid so we are not performing the extra step of verifying the script. However, in this situation, the finalizer has
...

🚀 achow101 merged a pull request: "test: delete commented-out tests and add a test case in wallet_signer"
(https://github.com/bitcoin/bitcoin/pull/33020)

💬 sipa commented on pull request "Enable `-natpmp` by default":
(https://github.com/bitcoin/bitcoin/pull/33004#issuecomment-3105053907)
utACK

💬 achow101 commented on pull request "init: [gui] Avoid UB/crash in InitAndLoadChainstate":
(https://github.com/bitcoin/bitcoin/pull/32987#issuecomment-3105070165)
ACK fac90e5261b811739ada56e06ea793a12f9c2c3d

📝 l0rinc opened a pull request: "doc: remove dangling toc entries from `developer-notes.md`"
(https://github.com/bitcoin/bitcoin/pull/33040)
The table of content wasn't adjusted when the corresponding entries were removed in https://github.com/bitcoin/bitcoin/pull/32572, see:
* `Ignoring IDE/editor files`: https://github.com/bitcoin/bitcoin/commit/faf65f05312be7647f485f088ba00fef97f47bf4
* `Shebang`: https://github.com/bitcoin/bitcoin/commit/7777fb8bc749e18c178ef460b65219187e676128
* `Wallet`: https://github.com/bitcoin/bitcoin/commit/fa69c5b170f56d554fcb0d0887bd27f961fe3e74