💬 sipa commented on issue "support BIP39 mnemonic in descriptors":
(https://github.com/bitcoin/bitcoin/issues/19151#issuecomment-2747930771)
I really don't see a problem with, or difficulty implementing, support for bip39 in descriptor parsing. Internally it would be mapped to an `xpub` instead, so `listdescriptors` etc wouldn't be able to echo the words back to you, but functionality-wise, that is no different from what #32115 achieved.
(https://github.com/bitcoin/bitcoin/issues/19151#issuecomment-2747930771)
I really don't see a problem with, or difficulty implementing, support for bip39 in descriptor parsing. Internally it would be mapped to an `xpub` instead, so `listdescriptors` etc wouldn't be able to echo the words back to you, but functionality-wise, that is no different from what #32115 achieved.
💬 ismaelsadeeq commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010067841)
fixed!
(https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2010067841)
fixed!
💬 sipa commented on issue "RFC: Compact Block Reconstruction Macro Benchmark Suite":
(https://github.com/bitcoin/bitcoin/issues/32131#issuecomment-2747938098)
The discussion at coredev that (I believe) led to this was focused on measuring the runtime of end-to-end block acceptance for 100% reconstructible blocks.
(https://github.com/bitcoin/bitcoin/issues/32131#issuecomment-2747938098)
The discussion at coredev that (I believe) led to this was focused on measuring the runtime of end-to-end block acceptance for 100% reconstructible blocks.
💬 ismaelsadeeq commented on pull request "doc: clarify the documentation of `Assume` assertion":
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2747939269)
> I find myself agreeing with the points mentioned in this comment [#32100 (comment)](https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2008737842), there is some redundancy in the verbiage currently that can be removed.
This is fixed now in [2898a0be..a7c65edc](https://github.com/bitcoin/bitcoin/compare/a880d1bf87817b1e6606c971cbfe98382898a0be..a7c65edc884b0e22aaabd7e725f5f39e60b6e76b)
(https://github.com/bitcoin/bitcoin/pull/32100#issuecomment-2747939269)
> I find myself agreeing with the points mentioned in this comment [#32100 (comment)](https://github.com/bitcoin/bitcoin/pull/32100#discussion_r2008737842), there is some redundancy in the verbiage currently that can be removed.
This is fixed now in [2898a0be..a7c65edc](https://github.com/bitcoin/bitcoin/compare/a880d1bf87817b1e6606c971cbfe98382898a0be..a7c65edc884b0e22aaabd7e725f5f39e60b6e76b)
💬 fjahr commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2748012184)
The intent is clearer to me now as well, thanks. I think the new test is valuable but I am not sure that this means that the old test has to be removed as well. These are two different tests now, the old test is testing a very simple off by one error and the new one is more elaborate. I don't really see why we have to throw away the first one.
The fear seems to be that the structure of the dump changes and that would require touching the test. I'm just not that concerned about that because th
...
(https://github.com/bitcoin/bitcoin/pull/32117#issuecomment-2748012184)
The intent is clearer to me now as well, thanks. I think the new test is valuable but I am not sure that this means that the old test has to be removed as well. These are two different tests now, the old test is testing a very simple off by one error and the new one is more elaborate. I don't really see why we have to throw away the first one.
The fear seems to be that the structure of the dump changes and that would require touching the test. I'm just not that concerned about that because th
...
💬 instagibbs commented on pull request "test: Add encodable PUSHDATA1 examples to feature_taproot":
(https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2748046346)
@fjahr I could do that, where the comment is literal examples that aren't an attempt to expand coverage. I think seeing it right at the top could be helpful
(https://github.com/bitcoin/bitcoin/pull/32114#issuecomment-2748046346)
@fjahr I could do that, where the comment is literal examples that aren't an attempt to expand coverage. I think seeing it right at the top could be helpful
💬 instagibbs commented on pull request "test: add missing segwitv1 test cases to `script_standard_tests`":
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010135316)
exactly, in case somehow the IsPayToAnchor definition diverged. It's a quick eye-ball to ensure correctness.
(https://github.com/bitcoin/bitcoin/pull/31340#discussion_r2010135316)
exactly, in case somehow the IsPayToAnchor definition diverged. It's a quick eye-ball to ensure correctness.
🤔 instagibbs reviewed a pull request: "test: add missing segwitv1 test cases to `script_standard_tests`"
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2710314205)
reACK 8284229a28c09c585356dcf7e4bddbc8f2a23755
(https://github.com/bitcoin/bitcoin/pull/31340#pullrequestreview-2710314205)
reACK 8284229a28c09c585356dcf7e4bddbc8f2a23755
🤔 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