🚀 fanquake merged a pull request: "allocators: Apply manual ASan poisoning to `PoolResource`"
(https://github.com/bitcoin/bitcoin/pull/32581)
(https://github.com/bitcoin/bitcoin/pull/32581)
🤔 danielabrozzoni reviewed a pull request: "cli: return local services in -netinfo"
(https://github.com/bitcoin/bitcoin/pull/31886#pullrequestreview-3087515075)
reACK 721a051320f2c10d2e9c89c985f180da81d64dca
(https://github.com/bitcoin/bitcoin/pull/31886#pullrequestreview-3087515075)
reACK 721a051320f2c10d2e9c89c985f180da81d64dca
👍 dergoegge approved a pull request: "Introduce per-txin sighash midstate cache for legacy/p2sh/segwitv0 scripts"
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3087710077)
Code review ACK fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c
(https://github.com/bitcoin/bitcoin/pull/32473#pullrequestreview-3087710077)
Code review ACK fb7e9decf3f12ebae786e0cecf2f24a91c6e9d4c
💬 waketraindev commented on pull request "qt: clear command history when clearing the console":
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3154660998)
Updated the patch to only clear input when Shift is held; also added Ctrl+Shift+L as a secondary shortcut.
(https://github.com/bitcoin-core/gui/pull/882#issuecomment-3154660998)
Updated the patch to only clear input when Shift is held; also added Ctrl+Shift+L as a secondary shortcut.
💬 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)