💬 RobinLinus commented on pull request "Reduce minrelaytxfee to 100 sats/kvB":
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3129732783)
> @RobinLinus, are you planning on working on this?
I'm interested in working on this, but I'm currently too busy moving apartments. I plan to revisit it in about two weeks. In the meantime, it would be greatly appreciated if someone could help fix the tests that fail when adjusting DEFAULT_BLOCK_MIN_TX_FEE.
(https://github.com/bitcoin/bitcoin/pull/32959#issuecomment-3129732783)
> @RobinLinus, are you planning on working on this?
I'm interested in working on this, but I'm currently too busy moving apartments. I plan to revisit it in about two weeks. In the meantime, it would be greatly appreciated if someone could help fix the tests that fail when adjusting DEFAULT_BLOCK_MIN_TX_FEE.
💬 l0rinc commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#issuecomment-3129749041)
Rebased after https://github.com/bitcoin/bitcoin/pull/32279 - updating the tests with the new convenience template helpers instead of using `Solver` for them. Ready for review again.
(https://github.com/bitcoin/bitcoin/pull/32729#issuecomment-3129749041)
Rebased after https://github.com/bitcoin/bitcoin/pull/32279 - updating the tests with the new convenience template helpers instead of using `Solver` for them. Ready for review again.
💬 purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3129870494)
I see in the C++ wrapper that a lot of functions are marked `noexcept`. I assume that the rationale is: "since this function just wraps a C function and C functions cannot throw, the function could just as well be marked `noexcept`".
But this is not how the keyword is meant to be used. The `noexcept` keyword is intended to be used on functions that should not be *allowed* to throw. The compiler then generates a runtime check to terminate the program in the case that the function throws. You c
...
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3129870494)
I see in the C++ wrapper that a lot of functions are marked `noexcept`. I assume that the rationale is: "since this function just wraps a C function and C functions cannot throw, the function could just as well be marked `noexcept`".
But this is not how the keyword is meant to be used. The `noexcept` keyword is intended to be used on functions that should not be *allowed* to throw. The compiler then generates a runtime check to terminate the program in the case that the function throws. You c
...
💬 l0rinc commented on pull request "[IBD] Tracking PR for speeding up Initial Block Download":
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3129873283)
Thanks for reviewing and reproducing https://github.com/bitcoin/bitcoin/pull/32279 - it's also merged 🎉
* https://github.com/bitcoin/bitcoin/pull/32497
* https://github.com/bitcoin/bitcoin/pull/30442
Reviews and ACKs for the above 2 remaining ones would be very welcome.
(https://github.com/bitcoin/bitcoin/pull/32043#issuecomment-3129873283)
Thanks for reviewing and reproducing https://github.com/bitcoin/bitcoin/pull/32279 - it's also merged 🎉
* https://github.com/bitcoin/bitcoin/pull/32497
* https://github.com/bitcoin/bitcoin/pull/30442
Reviews and ACKs for the above 2 remaining ones would be very welcome.
✅ l0rinc closed a pull request: "doc: Fix invalid txid in `gettransaction` RPC example"
(https://github.com/bitcoin/bitcoin/pull/31610)
(https://github.com/bitcoin/bitcoin/pull/31610)
💬 l0rinc commented on pull request "doc: Fix invalid txid in `gettransaction` RPC example":
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-3129900375)
Closing, it seems these invalid examples are on purpose - even though I think there are better ways to "prevent accidental transactions by users" than making the examples 63 characters long.
(https://github.com/bitcoin/bitcoin/pull/31610#issuecomment-3129900375)
Closing, it seems these invalid examples are on purpose - even though I think there are better ways to "prevent accidental transactions by users" than making the examples 63 characters long.
📝 darosior opened a pull request: "qa: test that we do not disconnect a peer for submitting an invalid compact block"
(https://github.com/bitcoin/bitcoin/pull/33083)
In thinking about https://github.com/bitcoin/bitcoin/pull/33050 and https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3111631541, i went through the code paths for peer disconnection upon submitting an invalid block. It turns out that the fact we exempt a peer from disconnection upon submitting an invalid compact block was not properly tested, as can be checked with these diffs:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index https://github.com/bitcoin/bitc
...
(https://github.com/bitcoin/bitcoin/pull/33083)
In thinking about https://github.com/bitcoin/bitcoin/pull/33050 and https://github.com/bitcoin/bitcoin/pull/33012#issuecomment-3111631541, i went through the code paths for peer disconnection upon submitting an invalid block. It turns out that the fact we exempt a peer from disconnection upon submitting an invalid compact block was not properly tested, as can be checked with these diffs:
```diff
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index https://github.com/bitcoin/bitc
...
💬 TheBlueMatt commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3129986454)
> It introduces a potential security issue though since we can't validate the output of the separate process. We're just trusting whatever it spits out, and if it were malicious, it could be making fake addresses which results in funds being sent to an attacker., and the user basically wouldn't know.
This is why I raised it in the context of multi-process - multi-process already inherently moves to a world where Bitcoin Core trusts output of other processes, doing this in that context wouldn'
...
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3129986454)
> It introduces a potential security issue though since we can't validate the output of the separate process. We're just trusting whatever it spits out, and if it were malicious, it could be making fake addresses which results in funds being sent to an attacker., and the user basically wouldn't know.
This is why I raised it in the context of multi-process - multi-process already inherently moves to a world where Bitcoin Core trusts output of other processes, doing this in that context wouldn'
...
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237910584)
Removed in e62bd1b24b6 ci: port tsan-depends-gui
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237910584)
Removed in e62bd1b24b6 ci: port tsan-depends-gui
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237906512)
I am actually not 100% sure why this is needed. Warp have it [documented in their docs](https://docs.warpbuild.com/cache/docker_layer_caching#step-1-set-up-docker-buildx-action), but IIRC Cirrus also advised us to set it.
My understanding is that it was necessary for the `gha` caching using custom URL, but perhaps @m3dwards or @fkorotkov could clarify/confirm?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237906512)
I am actually not 100% sure why this is needed. Warp have it [documented in their docs](https://docs.warpbuild.com/cache/docker_layer_caching#step-1-set-up-docker-buildx-action), but IIRC Cirrus also advised us to set it.
My understanding is that it was necessary for the `gha` caching using custom URL, but perhaps @m3dwards or @fkorotkov could clarify/confirm?
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237908036)
Added a helper job (runtime of ~4 seconds) which has an output whether or not to use cirrus to the subsequent jobs in 8758b6ec333 ci: add job to determine runner type
This reduces the hardcoding to a single env var in the main ci.yml file, which can trivially be patched by forks if desired.
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237908036)
Added a helper job (runtime of ~4 seconds) which has an output whether or not to use cirrus to the subsequent jobs in 8758b6ec333 ci: add job to determine runner type
This reduces the hardcoding to a single env var in the main ci.yml file, which can trivially be patched by forks if desired.
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237910261)
Removed dead code no-longer needed in 3825f903585 ci: port arm 32-bit job (and subsequent job commits)
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237910261)
Removed dead code no-longer needed in 3825f903585 ci: port arm 32-bit job (and subsequent job commits)
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237948012)
Set this to run on PRs in fd32aa85b29 ci: port lint by setting `CIRRUS_PR=1` via `if [ ${{ github.event_name }}" = "pull_request" ]; then` which I think should fix this?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237948012)
Set this to run on PRs in fd32aa85b29 ci: port lint by setting `CIRRUS_PR=1` via `if [ ${{ github.event_name }}" = "pull_request" ]; then` which I think should fix this?
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237950268)
Fixed, and now using `$PACKAGES` in 8bb213c64c3 ci: force reinstall of kernel headers in asan
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237950268)
Fixed, and now using `$PACKAGES` in 8bb213c64c3 ci: force reinstall of kernel headers in asan
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237909204)
Thanks, I agree.
Removed the input entirely and use `$CONTAINER_NAME` in d6ed832034b ci: add caching actions
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237909204)
Thanks, I agree.
Removed the input entirely and use `$CONTAINER_NAME` in d6ed832034b ci: add caching actions
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237935705)
Fixed in fd32aa85b29 ci: port lint
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237935705)
Fixed in fd32aa85b29 ci: port lint
💬 willcl-ark commented on pull request "Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237982299)
Removed the global makejobs in 89f177406c9 ci: dynamically match makejobs with cores
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2237982299)
Removed the global makejobs in 89f177406c9 ci: dynamically match makejobs with cores
🤔 jonatack reviewed a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3064816090)
Approach ACK
(https://github.com/bitcoin/bitcoin/pull/32866#pullrequestreview-3064816090)
Approach ACK
💬 jonatack commented on pull request "doc: add note for watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32866#discussion_r2238023764)
```suggestion
contains all of the watchonly scripts. `<name>_solvables` contains any scripts that the wallet
```
(https://github.com/bitcoin/bitcoin/pull/32866#discussion_r2238023764)
```suggestion
contains all of the watchonly scripts. `<name>_solvables` contains any scripts that the wallet
```
💬 jonatack commented on pull request "doc: add note for watch-only wallet migration":
(https://github.com/bitcoin/bitcoin/pull/32866#discussion_r2238023510)
```suggestion
wallets do not support having both private keys and watch-only scripts, there may be up to two
```
(https://github.com/bitcoin/bitcoin/pull/32866#discussion_r2238023510)
```suggestion
wallets do not support having both private keys and watch-only scripts, there may be up to two
```