💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1847043402)
I'd like to see another concept ACK to removing the comment I think before I do so, if someone else thinks it's valuable to drop the comment, I will.

💬 brunoerg commented on pull request "test: addrman: tried 3 times and never a success so `isTerrible=true`":
(https://github.com/bitcoin/bitcoin/pull/30445#discussion_r1847043582)
Note that `IsTerrible` has other verifications (e.g. we don't remove things tried in the last minute). So the timing setting here is to explicity reach exactly what we want to test but could be any other value. Every time we call `Attempt`, `m_last_try` is updated. So, yes, you're right about the threshold. Worth adding a comment?

💬 andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-2483780319)
I'm using the ThreadPool here in https://github.com/bitcoin/bitcoin/pull/31132 as a cherry-picked commit, modulo changing `ThreadPool() {}` to `ThreadPool() = default;`. Perhaps we could pull this out to a separate PR since it would be useful for both changes.

One request for the ThreadPool would be to track in flight tasks being executed. That way we could write tests that ensure that all tasks have been completed before continuing, even if we don't have access to the futures.

🤔 furszy reviewed a pull request: "refactor: Clamp worker threads in ChainstateManager constructor"
(https://github.com/bitcoin/bitcoin/pull/31313#pullrequestreview-2443430178)
Code ACK 8f85d36d68ab33ba237407a2ed16667eb149d61f

💬 mzumsande commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1847140617)
I think that this will change behavior in the case where we re-submit a previously pruned block with `submitblock`:

On master, we'd skip early here and return "duplicate" because the index is still `BLOCK_VALID_SCRIPTS`-valid.
But with his branch, we'd accept the block and save it to disk once more.

I'm really unsure if that is an undesired change in behavior or not - I guess it could also be viewed as a feature.
In any case, it's a behavior change that should be documented if we do go t
...

👍 ryanofsky approved a pull request: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963#pullrequestreview-2443280039)
Code review ACK 40143bafb52927eb40ecfde0ea722934213eb902.

This is a pretty simple change to the RPC framework that should make it easy to add new options to RPCs like [createwallet](https://github.com/bitcoin/bitcoin/pull/29129#pullrequestreview-1804913032) that currently accept too many positional parameters, without having to increase the number of positional parameters they already accept.

Without this PR, the only ways to add a new options to an RPC like `createwallet` without breaking
...

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1847054828)
In commit "RPC/Wallet: Convert walletprocesspsbt to use options parameter" (28023ff415ca1fa9c2b9c12e6bc525f8bc14aac5)

I think it would be good to move this code higher in the function, before the BlockUntilSyncedToCurrentChain and DecodeBase64PSBT calls to give caller more immediate feedback if they are calling this function with invalid parameters or the wrong types.

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1847064725)
In commit "RPC/Wallet: Convert walletprocesspsbt to use options parameter" (28023ff415ca1fa9c2b9c12e6bc525f8bc14aac5)

Might be good to test a combination of named / positional arguments like

```python
assert_equal(updated, self.nodes[1].walletprocesspsbt(psbt, sign=False, sighashtype='ALL', bip32derivs=True)["psbt"])
```

💬 ryanofsky commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1846994896)
re: https://github.com/bitcoin/bitcoin/pull/24963#discussion_r1470126512

> the github comment this is responding to was mostly about `RPCHelpMan::GetArgMap`, which can still be simplified.

Actually the simplification I suggested to `GetArgMap` won't work as long we want the option to be declared as "options|sign" instead of just "options", which I agree with Luke is less clear. So while my original suggestion was correct and simpler than current PR, I think the current approach is better,
...

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1847193982)
Added.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1847188474)
Added testcases flipping both descriptor order and blockhash order to ensure results don't change.

🤔 jamesob reviewed a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2443598792)
I've pushed changes addressing all outstanding feedback. I've slightly changed the return schema [as discussed on IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-11-15), moving all `scriptPubKey` related information into `output_spk`/`prevout_spk`, which matches the schema used in `getrawtransaction 2`.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1847219113)
Added a comment to this effect.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1847234154)
As discussed [on IRC](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-11-15), I've moved all sPK-specific info into a substructure that matches the one used in `getrawtransaction 2`. I've added @instagibbs' test under a commit with him as co-author.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1847218317)
Added tests including duplicate and reversal.

💬 jamesob commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1847250753)
Maybe could come as a follow-up?

🤔 furszy reviewed a pull request: "test: Introduce ensure_for helper"
(https://github.com/bitcoin/bitcoin/pull/30893#pullrequestreview-2443695664)
utACK 111465d72dd35e42361fc2a089036f652417ed37

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1847279135)
Good catch, thank you for taking another look! I'm also not sure what to do in light of this. At the minimum this reason for the duplicate check should be documented. I feel like with this kind of change I would err on the conservative side and not introduce potentially breaking behaviour. Even if this might be a nice way to inject block data into the node, I don't know if this is something anybody needs, even if it now more closely mimics `getblockfrompeer`.

We could keep the duplicate chec
...

💬 TheCharlatan commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1846964232)
Why is this not initialized again with `SCRIPT_ERR_UNKNOWN_ERROR`? I guess it is redundant, because it is set in the verification functions, but might still be good to keep it as a guard?

💬 TheCharlatan commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1847292854)
Isn't this the consensus only result?