π¬ maflcko commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380917171)
Would it help to set a new filename for the cached file, like `gh_$(package)-$($(package)_version).tar.gz`?
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380917171)
Would it help to set a new filename for the cached file, like `gh_$(package)-$($(package)_version).tar.gz`?
π¬ willcl-ark commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380937238)
Other workarounds may include (from least to most hacky):
- Revert this change on master and wait for a run to complete (and a new depends cache entry to be generated, to be picked up in 33559 and 33551)
- Acquire ability to delete caches from Cirrus (I have emailed @fkorotkov to ask if he can help us with this), after which we could do "uncached runs" on those branches
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3380937238)
Other workarounds may include (from least to most hacky):
- Revert this change on master and wait for a run to complete (and a new depends cache entry to be generated, to be picked up in 33559 and 33551)
- Acquire ability to delete caches from Cirrus (I have emailed @fkorotkov to ask if he can help us with this), after which we could do "uncached runs" on those branches
π¬ maflcko commented on pull request "ci: Check macos-cross executables on macOS":
(https://github.com/bitcoin/bitcoin/pull/33509#issuecomment-3380951117)
I just wonder if there ever was a single issue found if this was run in the CI in the past.
If this has never found an issue, the overhead doesn't seem worth it:
* It adds a 17 minute sequential spinner to the CI
* The new code requires maintenance
* The new task requires occasional manual re-runs due to https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2391749840
* GHA macOS VM provisioning may fail (seen twice in the last week), which requires even more manual re-runs
I can
...
(https://github.com/bitcoin/bitcoin/pull/33509#issuecomment-3380951117)
I just wonder if there ever was a single issue found if this was run in the CI in the past.
If this has never found an issue, the overhead doesn't seem worth it:
* It adds a 17 minute sequential spinner to the CI
* The new code requires maintenance
* The new task requires occasional manual re-runs due to https://github.com/bitcoin/bitcoin/pull/33303#discussion_r2391749840
* GHA macOS VM provisioning may fail (seen twice in the last week), which requires even more manual re-runs
I can
...
π¬ optout21 commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2413466183)
nit: "at a later time" is implied by "schedule", unnecessary to mention, plus it may suggest to the reader that you can control the later time here (which is not the case).
```suggestion
* Schedule a transaction for broadcasting to all peers.
```
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2413466183)
nit: "at a later time" is implied by "schedule", unnecessary to mention, plus it may suggest to the reader that you can control the later time here (which is not the case).
```suggestion
* Schedule a transaction for broadcasting to all peers.
```
π¬ optout21 commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2413467251)
nit: comma
```suggestion
* mempool, an `INV` about it is sent to the peer.
```
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2413467251)
nit: comma
```suggestion
* mempool, an `INV` about it is sent to the peer.
```
π¬ optout21 commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3380980623)
ACK 44a726133a880f818228c01b55236b3cb3eb1a67
Straightforward renaming of one method, plus doc comment change.
The change is rather small for a PR, but since it's part of a larger change (#29415), it's ok.
(https://github.com/bitcoin/bitcoin/pull/33565#issuecomment-3380980623)
ACK 44a726133a880f818228c01b55236b3cb3eb1a67
Straightforward renaming of one method, plus doc comment change.
The change is rather small for a PR, but since it's part of a larger change (#29415), it's ok.
π¬ hodlinator commented on pull request "Clear out space on GHA jobs":
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2413483771)
(Would be nice to clear this up since you switched back to this commit).
(https://github.com/bitcoin/bitcoin/pull/33514#discussion_r2413483771)
(Would be nice to clear this up since you switched back to this commit).
π¬ maflcko commented on pull request "ci: Add macOS cross task for arm64-apple-darwin":
(https://github.com/bitcoin/bitcoin/pull/33549#issuecomment-3380995526)
> Having at least compilation coverage of every target we are shipping a release binary for seems like something we should have? Post switchover we are much less constrained on the CI resources front.
Thx, done. Also, agree that cross-compilation is cheap and fast.
(https://github.com/bitcoin/bitcoin/pull/33549#issuecomment-3380995526)
> Having at least compilation coverage of every target we are shipping a release binary for seems like something we should have? Post switchover we are much less constrained on the CI resources front.
Thx, done. Also, agree that cross-compilation is cheap and fast.
π¬ Eunovo commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2413513497)
https://github.com/bitcoin/bitcoin/pull/26966/commits/0afb05cad29e4c30a2f9eb4295e997967129dc88
I wrote a test for recursive task submission. You can add it if you consider it to be useful.
```diff
diff --git a/src/test/threadpool_tests.cpp b/src/test/threadpool_tests.cpp
index ed0af8fa3f..4797144b78 100644
--- a/src/test/threadpool_tests.cpp
+++ b/src/test/threadpool_tests.cpp
@@ -197,6 +197,21 @@ BOOST_AUTO_TEST_CASE(threadpool_basic)
blocker.set_value();
threadPool
...
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2413513497)
https://github.com/bitcoin/bitcoin/pull/26966/commits/0afb05cad29e4c30a2f9eb4295e997967129dc88
I wrote a test for recursive task submission. You can add it if you consider it to be useful.
```diff
diff --git a/src/test/threadpool_tests.cpp b/src/test/threadpool_tests.cpp
index ed0af8fa3f..4797144b78 100644
--- a/src/test/threadpool_tests.cpp
+++ b/src/test/threadpool_tests.cpp
@@ -197,6 +197,21 @@ BOOST_AUTO_TEST_CASE(threadpool_basic)
blocker.set_value();
threadPool
...
π¬ Sjors commented on pull request "Clear out space on GHA jobs":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3381040718)
> Happy to revert this to only apply to the CentOS job if preferred. Seems pointless to add 10 mins runtime onto like 11 jobs where it's not needed...
While testing on with `sv2-tp` https://github.com/Sjors/sv2-tp/pull/37 I also noticed the cleanup task is quite slow.
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3381040718)
> Happy to revert this to only apply to the CentOS job if preferred. Seems pointless to add 10 mins runtime onto like 11 jobs where it's not needed...
While testing on with `sv2-tp` https://github.com/Sjors/sv2-tp/pull/37 I also noticed the cleanup task is quite slow.
β οΈ MRHUNG369 opened an issue: "bitcoin"
(https://github.com/bitcoin/bitcoin/issues/33572)
(https://github.com/bitcoin/bitcoin/issues/33572)
β
fanquake closed an issue: "bitcoin"
(https://github.com/bitcoin/bitcoin/issues/33572)
(https://github.com/bitcoin/bitcoin/issues/33572)
π¬ hebasto commented on pull request "[wip] A more static bitcoin-qt":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3381076578)
> > How is this approach better than #29923?
>
> They are doing different things. This PR produces a (much more) static `bitcoin-qt`, using depends as is. #29923 introduces new tooling, to essentially "hide" the runtime loading, the binary is not any more static.
Sure. But #29923 also reduces build dependencies. Isnβt that one of the goals of build system design?
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3381076578)
> > How is this approach better than #29923?
>
> They are doing different things. This PR produces a (much more) static `bitcoin-qt`, using depends as is. #29923 introduces new tooling, to essentially "hide" the runtime loading, the binary is not any more static.
Sure. But #29923 also reduces build dependencies. Isnβt that one of the goals of build system design?
π¬ Sjors commented on pull request "ci: Add macOS cross task for arm64-apple-darwin":
(https://github.com/bitcoin/bitcoin/pull/33549#issuecomment-3381079356)
utACK fad5a7101cc3dccbb525cfe9afc105aace8da88e
CI log suggests it worked.
(https://github.com/bitcoin/bitcoin/pull/33549#issuecomment-3381079356)
utACK fad5a7101cc3dccbb525cfe9afc105aace8da88e
CI log suggests it worked.
π¬ Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2413540219)
Think this is because the child processes have the same `g_rng_temp_path` so they create the same directory. Will need to see if `GetStrongRandBytes` introduces non-determinism. I'm confused why fuzzing on debian didn't bring this up.
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2413540219)
Think this is because the child processes have the same `g_rng_temp_path` so they create the same directory. Will need to see if `GetStrongRandBytes` introduces non-determinism. I'm confused why fuzzing on debian didn't bring this up.
π¬ hebasto commented on pull request "randomenv: Fix MinGW dllimport warning for `environ`":
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3381089487)
> Extends [7703884](https://github.com/bitcoin/bitcoin/commit/7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b) to guard environ declaration on all Windows builds, not just MSVC.
> MinGW also defines environ as a macro that expands to a dllimport function, causing the same inconsistent linkage warning.
I suppose this behaviour is specific to UCRT.
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3381089487)
> Extends [7703884](https://github.com/bitcoin/bitcoin/commit/7703884ab19cd7ffddc5c52ba57dec82fbc8dc2b) to guard environ declaration on all Windows builds, not just MSVC.
> MinGW also defines environ as a macro that expands to a dllimport function, causing the same inconsistent linkage warning.
I suppose this behaviour is specific to UCRT.
π¬ stickies-v commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2413568289)
I think there is potential for unconditional logging abuse here. If an inbound peer (which doesn't get disconnected for sending `BLOCK_CACHED_INVALID` headers) knows of (or spends the PoW to construct) an invalid block with a valid header, they can resend the same header and spam the log. The impacts should be limited since we now have disk filling mitigation in place, but it's still annoying at the very least, and it's very cheap to execute on the entire network when e.g. a miner accidentally c
...
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2413568289)
I think there is potential for unconditional logging abuse here. If an inbound peer (which doesn't get disconnected for sending `BLOCK_CACHED_INVALID` headers) knows of (or spends the PoW to construct) an invalid block with a valid header, they can resend the same header and spam the log. The impacts should be limited since we now have disk filling mitigation in place, but it's still annoying at the very least, and it's very cheap to execute on the entire network when e.g. a miner accidentally c
...
π¬ fanquake commented on pull request "[wip] A more static bitcoin-qt":
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3381125205)
> But https://github.com/bitcoin/bitcoin/pull/29923 also reduces build dependencies.
I'm not sure it reduces them; they are just moved into a different repository? That repository will need ongoing maintenance, the deps (headers) copied into there, as well as a [fake libc](https://github.com/laanwj/qtsowrap/tree/main/scripts/fake_libc_include), still need updating (I'm assuming syncronised with the Qt we are shipping in this repo, so changes needed there could also block changes here), as wel
...
(https://github.com/bitcoin/bitcoin/pull/33537#issuecomment-3381125205)
> But https://github.com/bitcoin/bitcoin/pull/29923 also reduces build dependencies.
I'm not sure it reduces them; they are just moved into a different repository? That repository will need ongoing maintenance, the deps (headers) copied into there, as well as a [fake libc](https://github.com/laanwj/qtsowrap/tree/main/scripts/fake_libc_include), still need updating (I'm assuming syncronised with the Qt we are shipping in this repo, so changes needed there could also block changes here), as wel
...
π¬ andrewtoth commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2413596564)
This would require `m_workers` to be synchronized somehow, since it is written in `Stop` and `Start` but read in `Submit`.
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2413596564)
This would require `m_workers` to be synchronized somehow, since it is written in `Stop` and `Start` but read in `Submit`.
π naiyoma's pull request is ready for review: "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value"
(https://github.com/bitcoin/bitcoin/pull/33498)
(https://github.com/bitcoin/bitcoin/pull/33498)