💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237716554)
Fixed in 526403df23a2db781709e4494da3a9f79284531d
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237716554)
Fixed in 526403df23a2db781709e4494da3a9f79284531d
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237717014)
Fixed in 526403df23a2db781709e4494da3a9f79284531d
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237717014)
Fixed in 526403df23a2db781709e4494da3a9f79284531d
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237718141)
Fixed in 526403df23a2db781709e4494da3a9f79284531d
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237718141)
Fixed in 526403df23a2db781709e4494da3a9f79284531d
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237719250)
Fixed in 65c8072757e58f9cad1198ddd8e403d656bb68e2
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237719250)
Fixed in 65c8072757e58f9cad1198ddd8e403d656bb68e2
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237727363)
Implemented in 526403df23a2db781709e4494da3a9f79284531d, but I think there is a CI failure now.
(https://github.com/bitcoin/bitcoin/pull/33011#discussion_r2237727363)
Implemented in 526403df23a2db781709e4494da3a9f79284531d, but I think there is a CI failure now.
💬 glozow commented on pull request "truc: optimize the in package relation calculation":
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2237728176)
Ah right, it actually doesn't help there
(https://github.com/bitcoin/bitcoin/pull/33062#discussion_r2237728176)
Ah right, it actually doesn't help there
💬 Crypt-iQ commented on pull request "log: rate limiting followups":
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3129226672)
The latest push 526403df23a2db781709e4494da3a9f79284531d:
- makes `m_limiter` a `std::shared_ptr`
- adds a commit to remove double-hashing in `SourceLocationHasher` (b8e92fb3d4137f91fe6a54829867fc54357da648)
- changes `logging_filesize_rate_limit` to dedupe logic and explicitly check for certain rate-limiting logs
The CI is [failing](https://api.cirrus-ci.com/v1/task/6153650155814912/logs/ci.log) with `DEBUG_LOCKORDER` as `ReadDebugLogLines` is matching against a log from the scheduler hav
...
(https://github.com/bitcoin/bitcoin/pull/33011#issuecomment-3129226672)
The latest push 526403df23a2db781709e4494da3a9f79284531d:
- makes `m_limiter` a `std::shared_ptr`
- adds a commit to remove double-hashing in `SourceLocationHasher` (b8e92fb3d4137f91fe6a54829867fc54357da648)
- changes `logging_filesize_rate_limit` to dedupe logic and explicitly check for certain rate-limiting logs
The CI is [failing](https://api.cirrus-ci.com/v1/task/6153650155814912/logs/ci.log) with `DEBUG_LOCKORDER` as `ReadDebugLogLines` is matching against a log from the scheduler hav
...
💬 l0rinc commented on pull request "test: revive test verifying that `GetCoinsCacheSizeState` switches from OK→LARGE→CRITICAL":
(https://github.com/bitcoin/bitcoin/pull/33021#issuecomment-3129437534)
Rebased, no other change needed, ready for review
(https://github.com/bitcoin/bitcoin/pull/33021#issuecomment-3129437534)
Rebased, no other change needed, ready for review
💬 sdaftuar commented on pull request "validation: docs and cleanups for MemPoolAccept coins views":
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3129561181)
utACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
(https://github.com/bitcoin/bitcoin/pull/32973#issuecomment-3129561181)
utACK b6d4688f77df9e31fd64e2be300f55bb8e944bd0
💬 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.