💬 fanquake commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3154666493)
if this is done, it should be added to `clang-tidy`:
```bash
[379/677][7.2s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/zmq/zmqrpc.cpp
775 warnings generated.
[380/677][10.0s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/coins_tests.cpp
/ci_container_base/src/test/coins_tests.cpp:397:48: error:
...
(https://github.com/bitcoin/bitcoin/pull/33094#issuecomment-3154666493)
if this is done, it should be added to `clang-tidy`:
```bash
[379/677][7.2s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/zmq/zmqrpc.cpp
775 warnings generated.
[380/677][10.0s] clang-tidy-20 -p=/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu -quiet -load=/tidy-build/libbitcoin-tidy.so /ci_container_base/src/test/coins_tests.cpp
/ci_container_base/src/test/coins_tests.cpp:397:48: error:
...
💬 0xB10C commented on pull request "cli: return local services in -netinfo":
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3154701807)
ACK 721a051320f2c10d2e9c89c985f180da81d64dca
light code-review and tested -netinfo
(also wondered if maintaining this as an external tool would be easier and allowed for faster iteration on it, but that's out of scope here)
(https://github.com/bitcoin/bitcoin/pull/31886#issuecomment-3154701807)
ACK 721a051320f2c10d2e9c89c985f180da81d64dca
light code-review and tested -netinfo
(also wondered if maintaining this as an external tool would be easier and allowed for faster iteration on it, but that's out of scope here)
💬 marcofleon commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-3154751797)
ACK eb68bb48ab2bb163f20dc7430d0401d88ca43f4a
Tested using `bitcoind` and the warning appears, although gets a bit lost among the output. There might be discussion I missed, but I don't see why we wouldn't just exit in this case. If the GUI warning is clear enough (as it seems like it is based on other reviews), then leaving the option to continue after the pop-up warning is probably fine. Either way, not a blocker, as having a warning is better than nothing.
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-3154751797)
ACK eb68bb48ab2bb163f20dc7430d0401d88ca43f4a
Tested using `bitcoind` and the warning appears, although gets a bit lost among the output. There might be discussion I missed, but I don't see why we wouldn't just exit in this case. If the GUI warning is clear enough (as it seems like it is based on other reviews), then leaving the option to continue after the pop-up warning is probably fine. Either way, not a blocker, as having a warning is better than nothing.
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254015767)
Dont need to worry about this since we are just removing the text
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254015767)
Dont need to worry about this since we are just removing the text
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254016722)
used an * instead as per @luke-jr 's suggestion
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254016722)
used an * instead as per @luke-jr 's suggestion
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254017528)
I did this slighly differently, please check it out
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254017528)
I did this slighly differently, please check it out
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254018302)
The new way feels cleaner than the recommended change
(https://github.com/bitcoin/bitcoin/pull/33126#discussion_r2254018302)
The new way feels cleaner than the recommended change
💬 Ataraxia009 commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3154768759)
> > to better support forked clients
>
> Concept NACK
>
> This is not something Bitcoin Core needs to support.
This objectively makes Core easier to fork and to maintain the fork.
It doesn't need to support it, but it does make it a more accessible open source project.
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3154768759)
> > to better support forked clients
>
> Concept NACK
>
> This is not something Bitcoin Core needs to support.
This objectively makes Core easier to fork and to maintain the fork.
It doesn't need to support it, but it does make it a more accessible open source project.
🤔 stratospher reviewed a pull request: "test: fix p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3087930833)
Nice detective work! Just 1 question, will ACK after that.
1. I tried some modifications of the test locally but wasn't able to enter the "else branch" where "tx was already announced to us". Were you able to test it?
2. Could also change MAX_REPEATS to a lower number. I'm getting `bad-txns-inputs-missingorspent` at run 47 when I remove the break in the loop. So don't think the test would work for full 100 runs anyways.
> Not sure if this is essential for the test, but we could add a rand
...
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3087930833)
Nice detective work! Just 1 question, will ACK after that.
1. I tried some modifications of the test locally but wasn't able to enter the "else branch" where "tx was already announced to us". Were you able to test it?
2. Could also change MAX_REPEATS to a lower number. I'm getting `bad-txns-inputs-missingorspent` at run 47 when I remove the break in the loop. So don't think the test would work for full 100 runs anyways.
> Not sure if this is essential for the test, but we could add a rand
...
📝 Sjors opened a pull request: "wallet: prevent accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135)
BIP 379 allows height and time locks that have no consensus meaning in BIP 68 / BIP 112. This is used by some protocols like Lightning to encode extra data, but is unsafe when used unintentionally. E.g. `older(65536)` is equivalent to `older(1)`.
This PR prevents accidental import of such descriptors. They will be rejected unless marked 'unsafe' in `importdescriptors`.
In preparation the first commit moves `FindNextChar` from `miniscript.{h,cpp}` to `parsing.{h,cpp}` which already contains
...
(https://github.com/bitcoin/bitcoin/pull/33135)
BIP 379 allows height and time locks that have no consensus meaning in BIP 68 / BIP 112. This is used by some protocols like Lightning to encode extra data, but is unsafe when used unintentionally. E.g. `older(65536)` is equivalent to `older(1)`.
This PR prevents accidental import of such descriptors. They will be rejected unless marked 'unsafe' in `importdescriptors`.
In preparation the first commit moves `FindNextChar` from `miniscript.{h,cpp}` to `parsing.{h,cpp}` which already contains
...
💬 Aris-Ritz commented on pull request "Removing Bitcoin core text where unnecessary":
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3154942267)
Test
(https://github.com/bitcoin/bitcoin/pull/33126#issuecomment-3154942267)
Test
📝 maflcko opened a pull request: "ci: Remove redundant RUN_UNIT_TESTS_SEQUENTIAL"
(https://github.com/bitcoin/bitcoin/pull/33136)
`RUN_UNIT_TESTS_SEQUENTIAL` is useful to detect cases where global state is left dirty in the test process and leads to subsequent unit test cases failing. However, one CI task is sufficient to catch this.
As there already is one, add docs there and remove this env var (and extra logic).
(https://github.com/bitcoin/bitcoin/pull/33136)
`RUN_UNIT_TESTS_SEQUENTIAL` is useful to detect cases where global state is left dirty in the test process and leads to subsequent unit test cases failing. However, one CI task is sufficient to catch this.
As there already is one, add docs there and remove this env var (and extra logic).
🤔 ryanofsky reviewed a pull request: "test,refactor: extract script template helpers & widen sigop count coverage"
(https://github.com/bitcoin/bitcoin/pull/32729#pullrequestreview-3087824321)
Code review f8208e92bd103359f8c3ceb3361eb7904099e994. Thanks for the updates, and I started reviewing the next chunk of this. Overall the changes seem helpful and well-considered and the extra test and benchmark coverage seem like they should be useful.
(https://github.com/bitcoin/bitcoin/pull/32729#pullrequestreview-3087824321)
Code review f8208e92bd103359f8c3ceb3361eb7904099e994. Thanks for the updates, and I started reviewing the next chunk of this. Overall the changes seem helpful and well-considered and the extra test and benchmark coverage seem like they should be useful.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254023250)
In commit "refactor: rename `GetSigOpCount` to `CountSigOps`" (4cbd303ddd7ae500683a7a595f38c5b8ff2de31e)
It's not obvious from commit message why this rename is being done. I think it might help to add some explanation like:
- Previous `GetSigOpCount` method was overloaded to take either a bool or scriptSig as a parameter, without an explanation of when to call each overload. New `CountSigOps` method avoids the overloading and documents how it should be called. The name was chosen to be cl
...
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254023250)
In commit "refactor: rename `GetSigOpCount` to `CountSigOps`" (4cbd303ddd7ae500683a7a595f38c5b8ff2de31e)
It's not obvious from commit message why this rename is being done. I think it might help to add some explanation like:
- Previous `GetSigOpCount` method was overloaded to take either a bool or scriptSig as a parameter, without an explanation of when to call each overload. New `CountSigOps` method avoids the overloading and documents how it should be called. The name was chosen to be cl
...
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254092329)
In commit "refactor: split off `P2SH` from `GetSigOpCount`" (37c1d8fc8db2f1ddd6ddf4d8fccbd13bbc9c9935)
Might be helpful to say what is motivating this change in the commit message. Maybe add:
- The name `CountP2SHSigOps` was chosen to match `CountWitnessSigOps`, since the two functions are counterparts for handling P2SH and SegWit scripts.
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254092329)
In commit "refactor: split off `P2SH` from `GetSigOpCount`" (37c1d8fc8db2f1ddd6ddf4d8fccbd13bbc9c9935)
Might be helpful to say what is motivating this change in the commit message. Maybe add:
- The name `CountP2SHSigOps` was chosen to match `CountWitnessSigOps`, since the two functions are counterparts for handling P2SH and SegWit scripts.
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254194990)
In commit "refactor: pull `IsPayToTaproot` to header" (38e89f646221ec0c15158b6ef0007336fec008e3)
Can commit message explain reason for other changes in this commit, like this test check and the IsWitnessProgram reordering above (assuming they are intentional)
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254194990)
In commit "refactor: pull `IsPayToTaproot` to header" (38e89f646221ec0c15158b6ef0007336fec008e3)
Can commit message explain reason for other changes in this commit, like this test check and the IsWitnessProgram reordering above (assuming they are intentional)
💬 ryanofsky commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254180036)
In commit "refactor: pull `IsPayToScriptHash` to header" (0d51088d1daa15ade3bf3c57410a0c12df235615)
It doesn't seem like WITNESS_V0_KEYHASH_SIZE is really the right constant to use for a p2sh script. Maybe should introduce a SCRIPT_HASH_SIZE or similar constant instead? Could also make sense to use the same constant for the memcpy in IsToScriptID
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2254180036)
In commit "refactor: pull `IsPayToScriptHash` to header" (0d51088d1daa15ade3bf3c57410a0c12df235615)
It doesn't seem like WITNESS_V0_KEYHASH_SIZE is really the right constant to use for a p2sh script. Maybe should introduce a SCRIPT_HASH_SIZE or similar constant instead? Could also make sense to use the same constant for the memcpy in IsToScriptID
📝 maflcko opened a pull request: "ci: Remove unused CI_FAILFAST_TEST_LEAVE_DANGLING"
(https://github.com/bitcoin/bitcoin/pull/33137)
`CI_FAILFAST_TEST_LEAVE_DANGLING` was added in commit 26d98d51f2cfc902df35f855590c052e16a5ce12 to run a bash `trap`. However, now that the `trap` was removed in commit 904631e0fc00ac9c8a03d1ce226d071bf88c00db, this can be removed as well.
If there is need for this in the future, it seems simpler to just explicitly execute `test_runner.py` in a new session via `setsid` instead of using this config/env juggling.
(https://github.com/bitcoin/bitcoin/pull/33137)
`CI_FAILFAST_TEST_LEAVE_DANGLING` was added in commit 26d98d51f2cfc902df35f855590c052e16a5ce12 to run a bash `trap`. However, now that the `trap` was removed in commit 904631e0fc00ac9c8a03d1ce226d071bf88c00db, this can be removed as well.
If there is need for this in the future, it seems simpler to just explicitly execute `test_runner.py` in a new session via `setsid` instead of using this config/env juggling.
💬 marcofleon commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2254283425)
fixed
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2254283425)
fixed
💬 marcofleon commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2254286215)
Left this one as is because `hash` seems to be used quite a bit throughout the gui and I don't want to change more code than I do currently.
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2254286215)
Left this one as is because `hash` seems to be used quite a bit throughout the gui and I don't want to change more code than I do currently.