🤔 l0rinc reviewed a pull request: "Draft: CCoinMap Experiments"
(https://github.com/bitcoin/bitcoin/pull/32128#pullrequestreview-2710273401)
Welcome back @martinus, we missed you! :)
I will measure these changes separately until 890k blocks soon.
I have included this change a tracking PR where we have other similar experiments: https://github.com/bitcoin/bitcoin/pull/32043
(https://github.com/bitcoin/bitcoin/pull/32128#pullrequestreview-2710273401)
Welcome back @martinus, we missed you! :)
I will measure these changes separately until 890k blocks soon.
I have included this change a tracking PR where we have other similar experiments: https://github.com/bitcoin/bitcoin/pull/32043
💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2010137339)
I have also measured this being a bottleneck very often (https://github.com/bitcoin/bitcoin/pull/30442), and @sipa mentioned originally that we can always swap it out with something better later: https://github.com/bitcoin/bitcoin/pull/8020#issuecomment-219569508
Do we need to account for collision attacks here, given that inclusion into the map is not for free?
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2010137339)
I have also measured this being a bottleneck very often (https://github.com/bitcoin/bitcoin/pull/30442), and @sipa mentioned originally that we can always swap it out with something better later: https://github.com/bitcoin/bitcoin/pull/8020#issuecomment-219569508
Do we need to account for collision attacks here, given that inclusion into the map is not for free?
💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2010125143)
This seems similar to https://github.com/bitcoin/bitcoin/pull/30326 - I'm surprised this has a measurable effect (and that I haven't found it so far :D)
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2010125143)
This seems similar to https://github.com/bitcoin/bitcoin/pull/30326 - I'm surprised this has a measurable effect (and that I haven't found it so far :D)
💬 l0rinc commented on pull request "Draft: CCoinMap Experiments":
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2010119858)
My understanding is that we're trying to get away from external dependencies, so if this does improve performance, we may want to copy it over.
For the record, similar attempts have been made before: https://github.com/bitcoin/bitcoin/pull/22640
@andrewtoth suggested we try your [`unordered_dense`](https://github.com/martinus/unordered_dense) here as well.
My understanding is that this will likely require retesting our memory related assumption so far.
(https://github.com/bitcoin/bitcoin/pull/32128#discussion_r2010119858)
My understanding is that we're trying to get away from external dependencies, so if this does improve performance, we may want to copy it over.
For the record, similar attempts have been made before: https://github.com/bitcoin/bitcoin/pull/22640
@andrewtoth suggested we try your [`unordered_dense`](https://github.com/martinus/unordered_dense) here as well.
My understanding is that this will likely require retesting our memory related assumption so far.
🤔 rkrux reviewed a pull request: "test: Add encodable PUSHDATA1 examples to feature_taproot"
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2710245464)
Concept ACK
Putting in my 2 sats here because I recently had the opportunity to go through this test
couple weeks back. Although it took me some time to understand the overall structure of the test but I was able to appreciate it soon after.
(https://github.com/bitcoin/bitcoin/pull/32114#pullrequestreview-2710245464)
Concept ACK
Putting in my 2 sats here because I recently had the opportunity to go through this test
couple weeks back. Although it took me some time to understand the overall structure of the test but I was able to appreciate it soon after.
💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2010104003)
I agree with this comment https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2746424182 that commentary like this can be put in a more "generic" place like the `Spender` tuple declaration here: https://github.com/bitcoin/bitcoin/blob/770d39a37652d40885533fecce37e9f71cc0d051/test/functional/feature_taproot.py#L465-L480
Something that lets the reader/tester quickly know in a generic way on how to use the `Spender` tuple, i.e.
1. make_spender
2. add_spender
3. test_spender
One mor
...
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2010104003)
I agree with this comment https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2746424182 that commentary like this can be put in a more "generic" place like the `Spender` tuple declaration here: https://github.com/bitcoin/bitcoin/blob/770d39a37652d40885533fecce37e9f71cc0d051/test/functional/feature_taproot.py#L465-L480
Something that lets the reader/tester quickly know in a generic way on how to use the `Spender` tuple, i.e.
1. make_spender
2. add_spender
3. test_spender
One mor
...
🤔 ajtowns requested changes to a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2710175333)
range-diff review of 41b4434fed169570ce0976c6e58db0d4a9614aaa
Good to see it's now explicit which calls you can make on an oversized txgraph.
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2710175333)
range-diff review of 41b4434fed169570ce0976c6e58db0d4a9614aaa
Good to see it's now explicit which calls you can make on an oversized txgraph.
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010063688)
Missing the word "cluster" (fixed in a later commit)
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010063688)
Missing the word "cluster" (fixed in a later commit)
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010068686)
Maybe clarify that each tx is included in its own list of ancestors/descendants?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010068686)
Maybe clarify that each tx is included in its own list of ancestors/descendants?
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010123251)
I don't think this is strictly true until the chunk indexes are added in #31444? Prior to that, it's possible to obtain such an ordering, but only by requesting all the clusters, breaking them into chunks manually (via chunk feerate calls on each tx in a cluster), then sorting the chunks.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010123251)
I don't think this is strictly true until the chunk indexes are added in #31444? Prior to that, it's possible to obtain such an ordering, but only by requesting all the clusters, breaking them into chunks manually (via chunk feerate calls on each tx in a cluster), then sorting the chunks.
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010139009)
Looks like comments for Exists/GetMainChunkFeerate/GetIndividualFeerate have been mixed up (in the "Add staging support" commit, I think)
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2010139009)
Looks like comments for Exists/GetMainChunkFeerate/GetIndividualFeerate have been mixed up (in the "Add staging support" commit, I think)
💬 instagibbs commented on pull request "fuzz: Fix off-by-one in package_rbf target":
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2748077309)
utACK fa9d9a33420c2db35e9ee7eebe5d6e475e26a8e8
change looks right
(https://github.com/bitcoin/bitcoin/pull/32122#issuecomment-2748077309)
utACK fa9d9a33420c2db35e9ee7eebe5d6e475e26a8e8
change looks right
💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2010150220)
I find that particular code block really tucked away and hard to find fwiw.
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2010150220)
I find that particular code block really tucked away and hard to find fwiw.
💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010152687)
```diff
diff --git a/test/functional/feature_checkcontractverify_vaults.py b/test/functional/feature_checkcontractverify_vaults.py
index 37e0bbd39f..60871850c1 100755
--- a/test/functional/feature_checkcontractverify_vaults.py
+++ b/test/functional/feature_checkcontractverify_vaults.py
@@ -47,19 +47,16 @@ class Vault(P2TR):
- with the "trigger" clause, sending the entire amount to an Unvaulting output, after providing a 'withdrawal_pk'
- with the "trigger_and_revault" clause, se
...
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010152687)
```diff
diff --git a/test/functional/feature_checkcontractverify_vaults.py b/test/functional/feature_checkcontractverify_vaults.py
index 37e0bbd39f..60871850c1 100755
--- a/test/functional/feature_checkcontractverify_vaults.py
+++ b/test/functional/feature_checkcontractverify_vaults.py
@@ -47,19 +47,16 @@ class Vault(P2TR):
- with the "trigger" clause, sending the entire amount to an Unvaulting output, after providing a 'withdrawal_pk'
- with the "trigger_and_revault" clause, se
...
👍 hodlinator approved a pull request: "doc: clarify the documentation of `Assume` assertion"
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2710323053)
ACK a7c65edc884b0e22aaabd7e725f5f39e60b6e76b
Adds more information on the strengths of `Assume()`, being able to be used as a kind of low-cost documentation of constraints that in the best case can be optimized away in production.
Latest version of the change also keeps with the pattern of only having one `- For example ...`-subsection.
(https://github.com/bitcoin/bitcoin/pull/32100#pullrequestreview-2710323053)
ACK a7c65edc884b0e22aaabd7e725f5f39e60b6e76b
Adds more information on the strengths of `Assume()`, being able to be used as a kind of low-cost documentation of constraints that in the best case can be optimized away in production.
Latest version of the change also keeps with the pattern of only having one `- For example ...`-subsection.
💬 hodlinator commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010147577)
Thank you for taking my suggestion!
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010147577)
Thank you for taking my suggestion!
💬 instagibbs commented on pull request "doc: Update comments for AreInputsStandard to match code":
(https://github.com/bitcoin/bitcoin/pull/32129#discussion_r2010168332)
It's still filtering other types of non-standard scripts, which also protects against the same issue of lots of operations (in addition to offering upgrade hooks)
(https://github.com/bitcoin/bitcoin/pull/32129#discussion_r2010168332)
It's still filtering other types of non-standard scripts, which also protects against the same issue of lots of operations (in addition to offering upgrade hooks)
💬 sipa commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2748126948)
ACK a7c65edc884b0e22aaabd7e725f5f39e60b6e76b
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2748126948)
ACK a7c65edc884b0e22aaabd7e725f5f39e60b6e76b
💬 rkrux commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2010180903)
True. I mentioned it because one would end up looking at it once they see `make_spender`. This place would be more useful if the `Spender` type was also used more in the test such as while initialising the `spenders` list.
I find right at the top a suitable alternative as well though I missed this section entirely the first time I read the test: https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_taproot.py#L121
Fine either way.
(https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2010180903)
True. I mentioned it because one would end up looking at it once they see `make_spender`. This place would be more useful if the `Spender` type was also used more in the test such as while initialising the `spenders` list.
I find right at the top a suitable alternative as well though I missed this section entirely the first time I read the test: https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_taproot.py#L121
Fine either way.
💬 brunoerg commented on pull request "fuzz: wallet: fix crypter target":
(https://github.com/bitcoin/bitcoin/pull/32118#issuecomment-2748136281)
> Would be nice to add a short line on how to test/observe this fix
I would say that simply checking the coverage would be enough. Note that https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/crypter.cpp.gcov.html L143 and L144 are unreachable and probably never be reach with the current harness.
(https://github.com/bitcoin/bitcoin/pull/32118#issuecomment-2748136281)
> Would be nice to add a short line on how to test/observe this fix
I would say that simply checking the coverage would be enough. Note that https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/crypter.cpp.gcov.html L143 and L144 are unreachable and probably never be reach with the current harness.