🤔 rkrux reviewed a pull request: "qa: make feature_assumeutxo.py test more robust"
(https://github.com/bitcoin/bitcoin/pull/32117#pullrequestreview-2710081823)
Concept ACK a7c57cf48a147c3ae3709f0630a3ba175aa4c841
This is much clearer compared to last time. I have reviewed this PR from the POV of someone new to the `assumeutxo` feature and asked a question because there appears to be two points that I have not been able to tie together yet.
(https://github.com/bitcoin/bitcoin/pull/32117#pullrequestreview-2710081823)
Concept ACK a7c57cf48a147c3ae3709f0630a3ba175aa4c841
This is much clearer compared to last time. I have reviewed this PR from the POV of someone new to the `assumeutxo` feature and asked a question because there appears to be two points that I have not been able to tie together yet.
💬 rkrux commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#discussion_r2010011383)
The comment says this is an invalid number of coins here but the validation code does actually read the said number of coins. Maybe mention the reason why the number of coins is invalid at this stage?
https://github.com/bitcoin/bitcoin/blob/770d39a37652d40885533fecce37e9f71cc0d051/src/validation.cpp#L5928-L5938
Mentioned this because the actual test failure error is caused by invalid height being passed in the serialized coin data, I'm trying to tie it up to why the number of coins is invali
...
(https://github.com/bitcoin/bitcoin/pull/32117#discussion_r2010011383)
The comment says this is an invalid number of coins here but the validation code does actually read the said number of coins. Maybe mention the reason why the number of coins is invalid at this stage?
https://github.com/bitcoin/bitcoin/blob/770d39a37652d40885533fecce37e9f71cc0d051/src/validation.cpp#L5928-L5938
Mentioned this because the actual test failure error is caused by invalid height being passed in the serialized coin data, I'm trying to tie it up to why the number of coins is invali
...
💬 rkrux commented on pull request "qa: make feature_assumeutxo.py test more robust":
(https://github.com/bitcoin/bitcoin/pull/32117#discussion_r2010025399)
```diff
-ser_varint(300 * 2)
+ser_varint((SNAPSHOT_BASE_HEIGHT + 1) * 2)
```
(https://github.com/bitcoin/bitcoin/pull/32117#discussion_r2010025399)
```diff
-ser_varint(300 * 2)
+ser_varint((SNAPSHOT_BASE_HEIGHT + 1) * 2)
```
💬 0xB10C commented on issue "RFC: Compact Block Reconstruction Macro Benchmark Suite":
(https://github.com/bitcoin/bitcoin/issues/32131#issuecomment-2747865062)
> Replaying compact block announcements and measuring reconstruction performance (multiple times for consistent and statistically meaningful results, given variability compared to stable micro-benchmarks)
What is the exact metric you are trying to measure? It's not 100% clear to me if you are trying to measure performance as in "speed" or performance as in "reconstructions without a round trip".
> Fetching the next few blocks from the network (lazy-init from network, caching the blocks locally
...
(https://github.com/bitcoin/bitcoin/issues/32131#issuecomment-2747865062)
> Replaying compact block announcements and measuring reconstruction performance (multiple times for consistent and statistically meaningful results, given variability compared to stable micro-benchmarks)
What is the exact metric you are trying to measure? It's not 100% clear to me if you are trying to measure performance as in "speed" or performance as in "reconstructions without a round trip".
> Fetching the next few blocks from the network (lazy-init from network, caching the blocks locally
...
💬 rkrux commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#discussion_r2010029902)
+1, I felt the same but I feel it's worth getting rid of duplication.
(https://github.com/bitcoin/bitcoin/pull/32116#discussion_r2010029902)
+1, I felt the same but I feel it's worth getting rid of duplication.
🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2709992001)
re-ACK 41b4434fed169570ce0976c6e58db0d4a9614aaa after https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2707582068
The new doc is better, and it's explicit that ref's can outlive their txgraph.
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2709992001)
re-ACK 41b4434fed169570ce0976c6e58db0d4a9614aaa after https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2707582068
The new doc is better, and it's explicit that ref's can outlive their txgraph.
💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2009970708)
As it is right now, it's unused in the main PR.
I've been thinking when `DoWork` can be called in the mempool lifecycle and whether that would make a difference because everything is done lazily and we only apply staged changes when we want to get the current state. (specifically a scenario where after some lazy additions it cheap to just call `DoWork` and not wait until we want to get a state.)
Maybe after trimming due to a reorg, after multiple additions of transactions e.g loading new
...
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2009970708)
As it is right now, it's unused in the main PR.
I've been thinking when `DoWork` can be called in the mempool lifecycle and whether that would make a difference because everything is done lazily and we only apply staged changes when we want to get the current state. (specifically a scenario where after some lazy additions it cheap to just call `DoWork` and not wait until we want to get a state.)
Maybe after trimming due to a reorg, after multiple additions of transactions e.g loading new
...
💬 l0rinc commented on issue "RFC: Macro Regression Test Suite for Historical Reorgs":
(https://github.com/bitcoin/bitcoin/issues/32130#issuecomment-2747870653)
Nice, we should do that as well!
I think it's important to be closer to the historical behavior as well - it may not be a tragedy if testnet behavior happens to change accidentally, but it is, if mainnet logic change isn't caught - that's why I insist on making it as realistic as possible.
(https://github.com/bitcoin/bitcoin/issues/32130#issuecomment-2747870653)
Nice, we should do that as well!
I think it's important to be closer to the historical behavior as well - it may not be a tragedy if testnet behavior happens to change accidentally, but it is, if mainnet logic change isn't caught - that's why I insist on making it as realistic as possible.
💬 l0rinc commented on pull request "contrib: refactor: dedup deserialization routines in utxo-to-sqlite script":
(https://github.com/bitcoin/bitcoin/pull/32116#discussion_r2010045532)
Though this means this utility file isn't as self-contained anymore (i.e. you can't just copy it out of the project - not even sure that was possible before)
(https://github.com/bitcoin/bitcoin/pull/32116#discussion_r2010045532)
Though this means this utility file isn't as self-contained anymore (i.e. you can't just copy it out of the project - not even sure that was possible before)
💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010016851)
In 0a1d149112af2835166e3d6e62a925df2e416e4e "test: Add functional tests for a vault construction based on CHECKCONTRACTVERIFY"
Can `alternate_pk` and `recover_pk` just be the same? That way there's only two key sets needed for a vault: one hot, one cold. And you're avoiding the use of a NUMS point.
BIP 345 (`OP_VAULT`) says:
> the recovery key can include a number of spending conditions, e.g. a time-delayed fallback to an "easier" recovery method, in case the highly secure key winds up
...
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2010016851)
In 0a1d149112af2835166e3d6e62a925df2e416e4e "test: Add functional tests for a vault construction based on CHECKCONTRACTVERIFY"
Can `alternate_pk` and `recover_pk` just be the same? That way there's only two key sets needed for a vault: one hot, one cold. And you're avoiding the use of a NUMS point.
BIP 345 (`OP_VAULT`) says:
> the recovery key can include a number of spending conditions, e.g. a time-delayed fallback to an "easier" recovery method, in case the highly secure key winds up
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2747921426)
1. Applied a new [patch](https://codereview.qt-project.org/c/qt/qtbase/+/634002) from Qt developers to avoid the `-no-pch` option for macOS.
2. `-no-opengl` has been moved from per-platform options to the global ones.
3. Rebased.
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2747921426)
1. Applied a new [patch](https://codereview.qt-project.org/c/qt/qtbase/+/634002) from Qt developers to avoid the `-no-pch` option for macOS.
2. `-no-opengl` has been moved from per-platform options to the global ones.
3. Rebased.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2010062215)
> It seems this patch is not enough...
[Fixed](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2747921426) for macOS as well.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r2010062215)
> It seems this patch is not enough...
[Fixed](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2747921426) for macOS as well.
💬 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