💬 stickies-v commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2394279590)
Oh I see. The fact that test functions are slightly renamed combined with this patch of code being ~duplicated tripped me up a bit. Thanks for the clarification, can be marked as resolved.
(https://github.com/bitcoin/bitcoin/pull/33476#discussion_r2394279590)
Oh I see. The fact that test functions are slightly renamed combined with this patch of code being ~duplicated tripped me up a bit. Thanks for the clarification, can be marked as resolved.
🤔 sipa reviewed a pull request: "validation: remove BLOCK_FAILED_CHILD"
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3288768556)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32950#pullrequestreview-3288768556)
Concept ACK
⚠️ venpisey12 opened an issue: "0968812058"
(https://github.com/bitcoin/bitcoin/issues/33513)
(https://github.com/bitcoin/bitcoin/issues/33513)
✅ willcl-ark closed an issue: "0968812058"
(https://github.com/bitcoin/bitcoin/issues/33513)
(https://github.com/bitcoin/bitcoin/issues/33513)
💬 sipa commented on issue "Mempool Expiry eviction might remove txs that could be mined in the next block":
(https://github.com/bitcoin/bitcoin/issues/33510#issuecomment-3356193889)
@davidgumberg I like the idea of expiring based on how many times a transaction was expected to be mined, but wasn't. E.g. every block, or every 10 minutes, or on a Poisson timer, run the block building code and increase a counter in the mempool for every transaction. Whenever the counter reaches a certain configurable value, expire. Note that that may well mean that nothing ever expires, if everything gets mined once it's spent enough time near the top of the mempool - but that may be ok, if it
...
(https://github.com/bitcoin/bitcoin/issues/33510#issuecomment-3356193889)
@davidgumberg I like the idea of expiring based on how many times a transaction was expected to be mined, but wasn't. E.g. every block, or every 10 minutes, or on a Poisson timer, run the block building code and increase a counter in the mempool for every transaction. Whenever the counter reaches a certain configurable value, expire. Note that that may well mean that nothing ever expires, if everything gets mined once it's spent enough time near the top of the mempool - but that may be ok, if it
...
💬 sipa commented on pull request "p2p: Use network-dependent timers for inbound inv scheduling":
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3356266112)
utACK 0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf
As a follow-up, I think we can give every outbound onion connection a separate network_id, because they all look distinct to external observers.
(https://github.com/bitcoin/bitcoin/pull/33464#issuecomment-3356266112)
utACK 0f7d4ee4e8281ed141a6ebb7e0edee7b864e4dcf
As a follow-up, I think we can give every outbound onion connection a separate network_id, because they all look distinct to external observers.
💬 sipa commented on pull request "[IBD] precalculate SipHash constant salt XORs":
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2394589493)
In commit "optimization: Cache PresaltedSipHasher in CBlockHeaderAndShortTxIDs"
Why is this optional? It looks like it's always initialized in the constructor.
(https://github.com/bitcoin/bitcoin/pull/30442#discussion_r2394589493)
In commit "optimization: Cache PresaltedSipHasher in CBlockHeaderAndShortTxIDs"
Why is this optional? It looks like it's always initialized in the constructor.
💬 m3dwards commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3356349768)
> But IMO exporting all variables prefixed with `ACTIONS_` to the CI env should not ever be problematic (and my insulate us a little in case new variables are introduced).
Which is what I think happened here to make this break in the first place so I'd support exporting all `ACTIONS_` vars.
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3356349768)
> But IMO exporting all variables prefixed with `ACTIONS_` to the CI env should not ever be problematic (and my insulate us a little in case new variables are introduced).
Which is what I think happened here to make this break in the first place so I'd support exporting all `ACTIONS_` vars.
💬 sipa commented on pull request "p2p: Mitigate GETADDR fingerprinting by setting address timestamps to a fixed value":
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3356355027)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33498#issuecomment-3356355027)
Concept ACK
🤔 sipa reviewed a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-3289213901)
utACK c9ffb6d7104f016ab89057bd57cd128ac03fd1fc
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-3289213901)
utACK c9ffb6d7104f016ab89057bd57cd128ac03fd1fc
💬 sipa commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2394630697)
In commit "net: avoid recursive m_nodes_mutex lock in DisconnectNode()"
Nit: the `auto` here made me worried this was making a copy of the `CNode` data structure. Maybe make the `CNode*` type explicit here?
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2394630697)
In commit "net: avoid recursive m_nodes_mutex lock in DisconnectNode()"
Nit: the `auto` here made me worried this was making a copy of the `CNode` data structure. Maybe make the `CNode*` type explicit here?
💬 willcl-ark commented on issue "[29.x] guix build failure on ppc with xcb":
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3356499067)
Ah I see.
FWIW I am using the same guix version via nix but can't reproduce :(
```
[100%] Built target check-symbols
-- Install configuration: "RelWithDebInfo"
-- Installing: /distsrc-base/distsrc-f6d49d0a0995-powerpc64-linux-gnu/installed/bitcoin-f6d49d0a0995/bin/bitcoin-wallet
-- Installing: /distsrc-base/distsrc-f6d49d0a0995-powerpc64-linux-gnu/installed/bitcoin-f6d49d0a0995/share/man/man1/bitcoin-wallet.1
-- Installing: /distsrc-base/distsrc-f6d49d0a0995-powerpc64-linux-gnu/installed/bitco
...
(https://github.com/bitcoin/bitcoin/issues/33488#issuecomment-3356499067)
Ah I see.
FWIW I am using the same guix version via nix but can't reproduce :(
```
[100%] Built target check-symbols
-- Install configuration: "RelWithDebInfo"
-- Installing: /distsrc-base/distsrc-f6d49d0a0995-powerpc64-linux-gnu/installed/bitcoin-f6d49d0a0995/bin/bitcoin-wallet
-- Installing: /distsrc-base/distsrc-f6d49d0a0995-powerpc64-linux-gnu/installed/bitcoin-f6d49d0a0995/share/man/man1/bitcoin-wallet.1
-- Installing: /distsrc-base/distsrc-f6d49d0a0995-powerpc64-linux-gnu/installed/bitco
...
📝 willcl-ark opened a pull request: "Clear out space on centos job"
(https://github.com/bitcoin/bitcoin/pull/33514)
Fixes #33293
Clear out space on the centos job be deleteing unnecessary files.
Raised in #33293 which pointed to a solution like https://github.com/google/oss-fuzz/commit/b7f04d782277638a67bc44865de445977eed4708 which is adapted slightly here.
Only runs when cache provider (runner) is `gha`, and on the CentOS job.
A run on my fork can be seen here: https://github.com/willcl-ark/bitcoin/actions/runs/18130629028/job/51596196163#step:6:2
(https://github.com/bitcoin/bitcoin/pull/33514)
Fixes #33293
Clear out space on the centos job be deleteing unnecessary files.
Raised in #33293 which pointed to a solution like https://github.com/google/oss-fuzz/commit/b7f04d782277638a67bc44865de445977eed4708 which is adapted slightly here.
Only runs when cache provider (runner) is `gha`, and on the CentOS job.
A run on my fork can be seen here: https://github.com/willcl-ark/bitcoin/actions/runs/18130629028/job/51596196163#step:6:2
💬 willcl-ark commented on pull request "Clear out space on centos job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356522227)
Maintaining an action in this repo which only caters to forks is not ideal, but unless GH increases the space on runners, or provides a "slim" image variant, then it might be the best there is for now.
As it doesn't run in this repo it should at least never break our CI.
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356522227)
Maintaining an action in this repo which only caters to forks is not ideal, but unless GH increases the space on runners, or provides a "slim" image variant, then it might be the best there is for now.
As it doesn't run in this repo it should at least never break our CI.
💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3356581222)
`0c718da693...73071b0cdb`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3356581222)
`0c718da693...73071b0cdb`: rebase due to conflicts
💬 katesalazar commented on pull request "Clear out space on centos job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356590750)
Which one is the chain needing this change?
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356590750)
Which one is the chain needing this change?
💬 janb84 commented on pull request "Clear out space on centos job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356595768)
So how does this relate to #33480 (remove centos for alpine) . Is this a fix until that PR gets merged ?
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356595768)
So how does this relate to #33480 (remove centos for alpine) . Is this a fix until that PR gets merged ?
💬 Raimo33 commented on pull request "validation: fetch block inputs on parallel threads >10% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3356601737)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31132#issuecomment-3356601737)
Concept ACK
💬 willcl-ark commented on pull request "Clear out space on centos job":
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356601782)
> Which one is the chain needing this change?
Ah I was slightly unclear; this actually applies to "personal forks" (e.g. my own, your own) as well as actual coin-forks. Anywhere where Cirrus runners, which have more space, are not being used.
> So how does this relate to #33480 (remove centos for alpine) . Is this a fix until that PR gets merged ?
I had thought the alpine job would use less space, but I think they are ~ equivalent, so this fix is still needed on that job for runs on for
...
(https://github.com/bitcoin/bitcoin/pull/33514#issuecomment-3356601782)
> Which one is the chain needing this change?
Ah I was slightly unclear; this actually applies to "personal forks" (e.g. my own, your own) as well as actual coin-forks. Anywhere where Cirrus runners, which have more space, are not being used.
> So how does this relate to #33480 (remove centos for alpine) . Is this a fix until that PR gets merged ?
I had thought the alpine job would use less space, but I think they are ~ equivalent, so this fix is still needed on that job for runs on for
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3356612313)
`3aa74b5176...22fadecc14`: rebase due to conflicts and pick changes from https://github.com/bitcoin/bitcoin/pull/33454
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3356612313)
`3aa74b5176...22fadecc14`: rebase due to conflicts and pick changes from https://github.com/bitcoin/bitcoin/pull/33454