💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1846977562)
I've discussed this question with @sipa a bit; our thought was that changesets are for evaluating changes that we might need to back out and not apply; removals due to a block being found or to make the mempool consistent after a reorg have to happen, so we don't bother with a changeset and just apply them directly.
If you have other thoughts on this let me know-- happy to reconsider if there's a good reason to structure removals differently.
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1846977562)
I've discussed this question with @sipa a bit; our thought was that changesets are for evaluating changes that we might need to back out and not apply; removals due to a block being found or to make the mempool consistent after a reorg have to happen, so we don't bother with a changeset and just apply them directly.
If you have other thoughts on this let me know-- happy to reconsider if there's a good reason to structure removals differently.
👍 tdb3 approved a pull request: "test: Revert to random path element"
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2443254703)
code review and light test ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3
Ran unit tests and benchmarks, and saw that the dirs generated were random.
(https://github.com/bitcoin/bitcoin/pull/31317#pullrequestreview-2443254703)
code review and light test ACK fa80b08fef0eaa600339caa678fdf80a8aec3ce3
Ran unit tests and benchmarks, and saw that the dirs generated were random.
👍 theStack approved a pull request: "Safegcd-based modular inverses in MuHash3072"
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2443257294)
Fuzz-tested ACK ad67fd2e0bfa6f43f350066596b6cca146391362
With the friendly help of @dergoegge I managed to get differential fuzzing running last week and let that ran for the last ~77 hours. Here are the rough instructions for those who also want to give it a try:
<details>
<summary>Instructions for differential fuzzing using semsan</summary>
1. created branches on top of master and the PR each that add a _characterization_ to the MuHash fuzz test, writing to a shared memory for comp
...
(https://github.com/bitcoin/bitcoin/pull/21590#pullrequestreview-2443257294)
Fuzz-tested ACK ad67fd2e0bfa6f43f350066596b6cca146391362
With the friendly help of @dergoegge I managed to get differential fuzzing running last week and let that ran for the last ~77 hours. Here are the rough instructions for those who also want to give it a try:
<details>
<summary>Instructions for differential fuzzing using semsan</summary>
1. created branches on top of master and the PR each that add a _characterization_ to the MuHash fuzz test, writing to a shared memory for comp
...
💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846980244)
> Calling this without the return value would just be removed as dead code.
Yes, it almost certainly would; but with [[nodiscard]] it simply won't compile. This is better as having some code such that calls ContainsNUL but doesn't use it, must be a bug, as there is no purpose to call it, and not use it.
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846980244)
> Calling this without the return value would just be removed as dead code.
Yes, it almost certainly would; but with [[nodiscard]] it simply won't compile. This is better as having some code such that calls ContainsNUL but doesn't use it, must be a bug, as there is no purpose to call it, and not use it.
💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#issuecomment-2483675312)
> Based on [#31301 (comment)](https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846027455), to set your expectations, this likely won't be merged (we can just postpone the fix until C++23, we're not in a rush)
I would argue against this mentality :) I can't tell for sure, but my guess is requiring c++23 is at least a year away from being merged? Makes more sense imo, to merge this in, and then once c++23 is available, update this code. (Also, I try to do PRs like this to move towards
...
(https://github.com/bitcoin/bitcoin/pull/31301#issuecomment-2483675312)
> Based on [#31301 (comment)](https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846027455), to set your expectations, this likely won't be merged (we can just postpone the fix until C++23, we're not in a rush)
I would argue against this mentality :) I can't tell for sure, but my guess is requiring c++23 is at least a year away from being merged? Makes more sense imo, to merge this in, and then once c++23 is available, update this code. (Also, I try to do PRs like this to move towards
...
💬 PastaPastaPasta commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846996998)
I would disagree, this way IDEs can show it as popup / doxygen can use it. Plus it is still accurate
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1846996998)
I would disagree, this way IDEs can show it as popup / doxygen can use it. Plus it is still accurate
👍 itornaza approved a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2443297256)
re ACK 6ffdabd4a00bf5578f975d0ed60c27adabf36c0b
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2443297256)
re ACK 6ffdabd4a00bf5578f975d0ed60c27adabf36c0b
💬 l0rinc commented on pull request "refactor: convert ContainsNoNUL to ContainsNUL":
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1847041055)
Given that you forgot to update it, I'd say it doesn't have a big added value, let's just delete it.
(https://github.com/bitcoin/bitcoin/pull/31301#discussion_r1847041055)
Given that you forgot to update it, I'd say it doesn't have a big added value, let's just delete it.
💬 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.
(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?
(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.
(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
(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
...
(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
...
(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.
(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"])
```
(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,
...
(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.
(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.
(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`.
(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`.