⚠️ fanquake opened an issue: "test: `interface_ipc.py` (might?) start skipping if installed capnp version changes"
(https://github.com/bitcoin/bitcoin/issues/34016)
Noticed this on a Fedora (Rawhide) Box that has capnp `1.2.0` (recently updated from 1.1.0) and `pycapnp` `2.1.0` installed, but was skipping `interface_ipc.py` as if the Python module was not available. The failure was that pycanp `2.1.0` tries to load an older shared lib, and fails:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py failed, Duration: 0 s
stdout:
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 2
...
(https://github.com/bitcoin/bitcoin/issues/34016)
Noticed this on a Fedora (Rawhide) Box that has capnp `1.2.0` (recently updated from 1.1.0) and `pycapnp` `2.1.0` installed, but was skipping `interface_ipc.py` as if the Python module was not available. The failure was that pycanp `2.1.0` tries to load an older shared lib, and fails:
```bash
Remaining jobs: [interface_ipc.py]
1/1 - interface_ipc.py failed, Duration: 0 s
stdout:
stderr:
Traceback (most recent call last):
File "/root/ci_scratch/build/test/functional/interface_ipc.py", line 2
...
👍 ryanofsky approved a pull request: "Add util::Expected (std::expected)"
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3545066334)
Code review ACK fa76a6620012fb738639d8fd7ce17b185bfd376c
(https://github.com/bitcoin/bitcoin/pull/34006#pullrequestreview-3545066334)
Code review ACK fa76a6620012fb738639d8fd7ce17b185bfd376c
🤔 darosior reviewed a pull request: "test: Add DERSIG unit tests to script_tests.json"
(https://github.com/bitcoin/bitcoin/pull/33973#pullrequestreview-3545073630)
Looks good to me, just want to confirm my understanding of one of the test cases.
(https://github.com/bitcoin/bitcoin/pull/33973#pullrequestreview-3545073630)
Looks good to me, just want to confirm my understanding of one of the test cases.
💬 darosior commented on pull request "test: Add DERSIG unit tests to script_tests.json":
(https://github.com/bitcoin/bitcoin/pull/33973#discussion_r2592933311)
So here the correctly-DER-encoded signature is not a valid signature for the correctly-encoded public key, is that right?
(https://github.com/bitcoin/bitcoin/pull/33973#discussion_r2592933311)
So here the correctly-DER-encoded signature is not a valid signature for the correctly-encoded public key, is that right?
💬 hodlinator commented on pull request "Add util::Expected (std::expected)":
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592946847)
nit: Not a fan of exceptions, and agree that it is a logic error to call `value()` or `error()` without first checking the state. But I think it might be slightly safer to `throw` here and below instead of asserting, so we have less of a behavior change once `util::Expected` is replaced by `std::expected`.
Otherwise users might experience changes under rare/improbable conditions where the process would shut down/crash before, and after the switch it would catch the exception and continue.
(https://github.com/bitcoin/bitcoin/pull/34006#discussion_r2592946847)
nit: Not a fan of exceptions, and agree that it is a logic error to call `value()` or `error()` without first checking the state. But I think it might be slightly safer to `throw` here and below instead of asserting, so we have less of a behavior change once `util::Expected` is replaced by `std::expected`.
Otherwise users might experience changes under rare/improbable conditions where the process would shut down/crash before, and after the switch it would catch the exception and continue.
🚀 fanquake merged a pull request: "[30.x] Backports & 30.1rc1"
(https://github.com/bitcoin/bitcoin/pull/33997)
(https://github.com/bitcoin/bitcoin/pull/33997)
👍 darosior approved a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3545098441)
utACK 9d5021a05bd33c73276909eec961777867ddb412
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3545098441)
utACK 9d5021a05bd33c73276909eec961777867ddb412
💬 fanquake commented on issue "Minor Release 30.1":
(https://github.com/bitcoin/bitcoin/issues/34015#issuecomment-3617242317)
`v30.1rc1` tag is available: https://github.com/bitcoin/bitcoin/compare/v30.1rc1.
(https://github.com/bitcoin/bitcoin/issues/34015#issuecomment-3617242317)
`v30.1rc1` tag is available: https://github.com/bitcoin/bitcoin/compare/v30.1rc1.
👍 darosior approved a pull request: "Fix 11-year-old mis-categorized error code in OP_IF evaluation"
(https://github.com/bitcoin/bitcoin/pull/32143#pullrequestreview-3545111160)
utACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
(https://github.com/bitcoin/bitcoin/pull/32143#pullrequestreview-3545111160)
utACK a7b581423e44c51fb7d177c5a15fe2cc2ab8aa43
🤔 sipa reviewed a pull request: "script: Add a separate ScriptError for empty pubkeys encountered in Tapscript"
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3545113694)
ACK 9d5021a05bd33c73276909eec961777867ddb412
(https://github.com/bitcoin/bitcoin/pull/33961#pullrequestreview-3545113694)
ACK 9d5021a05bd33c73276909eec961777867ddb412
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617315125)
I've generated some flamegraphs from perf data recorded during IBD for ~10k blocks between 850-900k for both master and this branch.
The 4 worker threads can be seen clearly on the left of the graph. The binary search through short txids is barely noticeable, which confirms our approach to not use an unordered_set + salted hasher.
Looking at `b-msghand` (main) thread, we see `ConnectBlock` dominating it on master while block serialization becomes the dominant factor on this branch. The mai
...
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3617315125)
I've generated some flamegraphs from perf data recorded during IBD for ~10k blocks between 850-900k for both master and this branch.
The 4 worker threads can be seen clearly on the left of the graph. The binary search through short txids is barely noticeable, which confirms our approach to not use an unordered_set + salted hasher.
Looking at `b-msghand` (main) thread, we see `ConnectBlock` dominating it on master while block serialization becomes the dominant factor on this branch. The mai
...
🤔 furszy reviewed a pull request: "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)"
(https://github.com/bitcoin/bitcoin/pull/33186#pullrequestreview-3545108565)
> The node v0.14.3 cannot be synced because it doesn't have the syncwithvalidationinterfacequeue RPC implemented (required by test_framework.py), so the solution is to copy the block folder to the modern node and start it with -reindex-chainstate. Because of this additional complexity, this testing is best managed in a separate file rather than in the existing migration test files.
In order to know when the node is synced, cannot you just wait for a specific log line? See `busy_wait_for_debug
...
(https://github.com/bitcoin/bitcoin/pull/33186#pullrequestreview-3545108565)
> The node v0.14.3 cannot be synced because it doesn't have the syncwithvalidationinterfacequeue RPC implemented (required by test_framework.py), so the solution is to copy the block folder to the modern node and start it with -reindex-chainstate. Because of this additional complexity, this testing is best managed in a separate file rather than in the existing migration test files.
In order to know when the node is synced, cannot you just wait for a specific log line? See `busy_wait_for_debug
...
💬 furszy commented on pull request "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)":
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2593023838)
Could check the wallet version here.
The non-hd one should have version 6000 (`WalletFeature::FEATURE_COMPRPUBKEY`) and the hd one should have version 130000 (`WalletFeature::FEATURE_HD`)
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2593023838)
Could check the wallet version here.
The non-hd one should have version 6000 (`WalletFeature::FEATURE_COMPRPUBKEY`) and the hd one should have version 130000 (`WalletFeature::FEATURE_HD`)
💬 furszy commented on pull request "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)":
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2593032651)
rescanning after migration shouldn't be needed
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2593032651)
rescanning after migration shouldn't be needed
💬 furszy commented on pull request "wallet, test: Ancient Wallet Migration from v0.14.3 (no-HD and Single Chain)":
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2592962526)
@w0xlt can you please check this ^^ ?
Also, aren't we essentially removing the unicode coverage from the CI with this change? `wallet_ancient_migration.py` test should always be enabled there.
(https://github.com/bitcoin/bitcoin/pull/33186#discussion_r2592962526)
@w0xlt can you please check this ^^ ?
Also, aren't we essentially removing the unicode coverage from the CI with this change? `wallet_ancient_migration.py` test should always be enabled there.
💬 jurraca commented on pull request "contrib: Count entry differences in asmap-tool diff summary":
(https://github.com/bitcoin/bitcoin/pull/33939#issuecomment-3617365793)
utACK `fd4ce55121`
tested several different diff runs.
(https://github.com/bitcoin/bitcoin/pull/33939#issuecomment-3617365793)
utACK `fd4ce55121`
tested several different diff runs.
🚀 fanquake merged a pull request: "contrib: Count entry differences in asmap-tool diff summary"
(https://github.com/bitcoin/bitcoin/pull/33939)
(https://github.com/bitcoin/bitcoin/pull/33939)
💬 ryanofsky commented on pull request "refactor: Add util::Result failure types and ability to merge result values":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617394520)
re: https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617021123
Thanks for taking a look at this.
> I think `util::Result` is conflating 2 related but orthogonal needs:
>
> * Returning a successful value or error(s).
> * Returning multiple structured errors/warnings, not just one.
The result class is handling both of these things (since c++ works most conveniently with a single return value), but I don't see a sense in which it is conflating them. The implementation handle
...
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617394520)
re: https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3617021123
Thanks for taking a look at this.
> I think `util::Result` is conflating 2 related but orthogonal needs:
>
> * Returning a successful value or error(s).
> * Returning multiple structured errors/warnings, not just one.
The result class is handling both of these things (since c++ works most conveniently with a single return value), but I don't see a sense in which it is conflating them. The implementation handle
...
💬 rkrux commented on pull request "wallet: check for `agg_pub` validity in MuSig2 signing":
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3617404481)
I did think about adding it there but decided against it to keep the change related to the crash site. But I don't feel strongly about it and will update the PR to add the check in PSBT deserialization.
(https://github.com/bitcoin/bitcoin/pull/34010#issuecomment-3617404481)
I did think about adding it there but decided against it to keep the change related to the crash site. But I don't feel strongly about it and will update the PR to add the check in PSBT deserialization.
📝 Chand-ra opened a pull request: "fuzz: Add a test case for `ParseByteUnits()`"
(https://github.com/bitcoin/bitcoin/pull/34017)
`ParseByteUnits()` is the only parsing function in `strencodings.cpp` lacking a fuzz test. Add a test case to check the function against arbitrary strings and randomized `default_multiplier`.
(https://github.com/bitcoin/bitcoin/pull/34017)
`ParseByteUnits()` is the only parsing function in `strencodings.cpp` lacking a fuzz test. Add a test case to check the function against arbitrary strings and randomized `default_multiplier`.