📝 maflcko opened a pull request: "ci: Lint follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33776)
This contains a few follow-ups to https://github.com/bitcoin/bitcoin/pull/33744:
* Rewrite the actions Bash snippet to Python. I've confirmed it still works in https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/19067932430 (scroll down).
* Add a lint-build retry to avoid issues such as https://github.com/bitcoin/bitcoin/issues/33640 for the lint task as well.
* Finally, run the `debug_pull_request_number_str` annotation on all checks, to ensure they are present even when GitHub de
...
(https://github.com/bitcoin/bitcoin/pull/33776)
This contains a few follow-ups to https://github.com/bitcoin/bitcoin/pull/33744:
* Rewrite the actions Bash snippet to Python. I've confirmed it still works in https://github.com/maflcko/bitcoin-core-with-ci/actions/runs/19067932430 (scroll down).
* Add a lint-build retry to avoid issues such as https://github.com/bitcoin/bitcoin/issues/33640 for the lint task as well.
* Finally, run the `debug_pull_request_number_str` annotation on all checks, to ensure they are present even when GitHub de
...
💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3486049487)
https://github.com/bitcoin/bitcoin/commit/51d213d1449b61665628ce9b4e0209cff3c4f818#r169601405 left a comment in the non-PR review mode, just making sure it gets noticed
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3486049487)
https://github.com/bitcoin/bitcoin/commit/51d213d1449b61665628ce9b4e0209cff3c4f818#r169601405 left a comment in the non-PR review mode, just making sure it gets noticed
💬 laanwj commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486050199)
Code review ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969
i think this is a good start for the C API, and ready for merge.
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3486050199)
Code review ACK 6c7a34f3b0bd39ef7a1520aac56e12f78e5cc969
i think this is a good start for the C API, and ready for merge.
💬 TheBlueMatt commented on issue "Batch tx broadcast RPC":
(https://github.com/bitcoin/bitcoin/issues/33700#issuecomment-3486063102)
> What would you prefer to happen if a transaction fails? Is it fine if we quit altogether or do you want the node to keep trying with everything else?
Parameter as mentioned by @ajtowns would be fine, but my general view of transaction relay is its all fallible anyway (nodes may be partitioned/fees may spike/we may be disconnected from the internet/etc/etc) so I want to just send a bunch of transaction hex into the void and Bitcoin Core is responsible for getting as much as possible confirmed.
...
(https://github.com/bitcoin/bitcoin/issues/33700#issuecomment-3486063102)
> What would you prefer to happen if a transaction fails? Is it fine if we quit altogether or do you want the node to keep trying with everything else?
Parameter as mentioned by @ajtowns would be fine, but my general view of transaction relay is its all fallible anyway (nodes may be partitioned/fees may spike/we may be disconnected from the internet/etc/etc) so I want to just send a bunch of transaction hex into the void and Bitcoin Core is responsible for getting as much as possible confirmed.
...
💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3486069744)
@ajtowns, thanks for looking into this. Some replies inline:
> Not sure the internal implementation around `BroadcastTransaction()` fully makes sense -- I could see `BroadcastTransaction()` having the knowledge of how transactions should be broadcast being better than having every caller of it specify it.
In `master` it is the latter - callers of `BroadcastTransaction()` tell it whether to broadcast and/or put in the mempool. I guess this can be changed, at least for the private broadcast
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3486069744)
@ajtowns, thanks for looking into this. Some replies inline:
> Not sure the internal implementation around `BroadcastTransaction()` fully makes sense -- I could see `BroadcastTransaction()` having the knowledge of how transactions should be broadcast being better than having every caller of it specify it.
In `master` it is the latter - callers of `BroadcastTransaction()` tell it whether to broadcast and/or put in the mempool. I guess this can be changed, at least for the private broadcast
...
💬 pinheadmz commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3486092759)
> While I no longer support the Bitcoin Core project
@luke-jr I think you should open a PR to remove your seed for this reason. It would be much more reasonable than someone else opening the PR on your behalf.
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3486092759)
> While I no longer support the Bitcoin Core project
@luke-jr I think you should open a PR to remove your seed for this reason. It would be much more reasonable than someone else opening the PR on your behalf.
👍 dergoegge approved a pull request: "ci: run native fuzz with MSAN job"
(https://github.com/bitcoin/bitcoin/pull/33626#pullrequestreview-3416571878)
utACK 1e6e32fa8a64daa21c9c9de437f7a12745ed4a4e
(https://github.com/bitcoin/bitcoin/pull/33626#pullrequestreview-3416571878)
utACK 1e6e32fa8a64daa21c9c9de437f7a12745ed4a4e
💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2490601023)
I'd rather we use the version of the compiler with more bug fixes, and less known bugs.
(https://github.com/bitcoin/bitcoin/pull/33775#discussion_r2490601023)
I'd rather we use the version of the compiler with more bug fixes, and less known bugs.
💬 luke-jr commented on issue "dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us appears to be violating DNS seed policy":
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3486116400)
Don't twist my quote out of context. Removing my DNS seed would be no harm to me, but it still shouldn't be done and would reflect poorly on Bitcoin Core if you do it anyway.
(https://github.com/bitcoin/bitcoin/issues/33734#issuecomment-3486116400)
Don't twist my quote out of context. Removing my DNS seed would be no harm to me, but it still shouldn't be done and would reflect poorly on Bitcoin Core if you do it anyway.
💬 maflcko commented on pull request "ci: run native fuzz with MSAN job":
(https://github.com/bitcoin/bitcoin/pull/33626#issuecomment-3486123197)
lgtm ACK 1e6e32fa8a64daa21c9c9de437f7a12745ed4a4e
(https://github.com/bitcoin/bitcoin/pull/33626#issuecomment-3486123197)
lgtm ACK 1e6e32fa8a64daa21c9c9de437f7a12745ed4a4e
💬 hebasto commented on pull request "cmake: Move IPC tests to `ipc/test`":
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3486133274)
I added a commit with further improvements.
(https://github.com/bitcoin/bitcoin/pull/33774#issuecomment-3486133274)
I added a commit with further improvements.
💬 fanquake commented on pull request "ci: Lint follow-ups":
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3486136927)
cc @willcl-ark @m3dwards
(https://github.com/bitcoin/bitcoin/pull/33776#issuecomment-3486136927)
cc @willcl-ark @m3dwards
🚀 fanquake merged a pull request: "ci: run native fuzz with MSAN job"
(https://github.com/bitcoin/bitcoin/pull/33626)
(https://github.com/bitcoin/bitcoin/pull/33626)
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3486197237)
`69c015b258...6091e94750`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3486197237)
`69c015b258...6091e94750`: rebase due to conflicts
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2490689248)
Can we do it for the shorter ones as well? it's easier to compare them and more consistent with the rest
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2490689248)
Can we do it for the shorter ones as well? it's easier to compare them and more consistent with the rest
💬 laanwj commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-3486217821)
> A year later, adoption is now almost at ~70% according to https://21.ninja/reachable-nodes/node-service-share/ - could update the OP with that info.
Concept ACK. i was critical of this in the past, but i'm more convinced that it makes some sense now, and believe it outweighs the implementation/maintenance cost of such a feature.
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-3486217821)
> A year later, adoption is now almost at ~70% according to https://21.ninja/reachable-nodes/node-service-share/ - could update the OP with that info.
Concept ACK. i was critical of this in the past, but i'm more convinced that it makes some sense now, and believe it outweighs the implementation/maintenance cost of such a feature.
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2490696786)
The test became more complicated than it was before (which was my main critique of it, seems arbitrary).
Could we simplify it? I think we're testing too many things, a single median check should likely suffice in my opinion.
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2490696786)
The test became more complicated than it was before (which was my main critique of it, seems arbitrary).
Could we simplify it? I think we're testing too many things, a single median check should likely suffice in my opinion.
💬 l0rinc commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2490699922)
what does `--` mean here?
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2490699922)
what does `--` mean here?
💬 frankomosh commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2490694848)
Only 1 coin is created (~1 BTC), but the target is 8 BTC. So my superficial thought is that `result_b` might never reach the target, therefore, the resulting failure would not be because of `TOTAL_TRIES` being hit, but simply insufficient funds. Or might I be missing some context here?
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2490694848)
Only 1 coin is created (~1 BTC), but the target is 8 BTC. So my superficial thought is that `result_b` might never reach the target, therefore, the resulting failure would not be because of `TOTAL_TRIES` being hit, but simply insufficient funds. Or might I be missing some context here?
💬 frankomosh commented on pull request "test: add case where `TOTAL_TRIES` is exceeded yet solution remains":
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2490695744)
nit: I think there's a double space after `=`. (maybe should be single spaced for consistency?)
(https://github.com/bitcoin/bitcoin/pull/33701#discussion_r2490695744)
nit: I think there's a double space after `=`. (maybe should be single spaced for consistency?)