Bitcoin Core Github
43 subscribers
122K links
Download Telegram
⚠️ fanquake reopened an issue: "CI: Improve documentation around replicating CI locally"
(https://github.com/bitcoin/bitcoin/issues/31199)
I've received some feedback that replicating CI jobs locally isn't straightforward. Perhaps the documentation can be improved in this area?

@maflcko I understand you had some improvements in mind? Happy to also work on this.
πŸ’¬ fanquake commented on issue "CI: Improve documentation around replicating CI locally":
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3559214112)
@m3dwards can you advise if this can be closed after #33887. Or clarify in the description what else should be done.
πŸ“ hebasto opened a pull request: "depends: Update Qt download link"
(https://github.com/bitcoin/bitcoin/pull/33918)
Replace the [unreliable](https://github.com/bitcoin/bitcoin/issues/33898#issuecomment-3559092421) https://code.qt.io with the GitHub mirror link.

Closes https://github.com/bitcoin/bitcoin/issues/33898.
βœ… hebasto closed a pull request: "cmake: Fix `-pthread` flags presentation in summary"
(https://github.com/bitcoin/bitcoin/pull/31724)
πŸ’¬ fanquake commented on pull request "depends: Update Qt download link":
(https://github.com/bitcoin/bitcoin/pull/33918#issuecomment-3559220586)
Concept ACK
πŸš€ fanquake merged a pull request: "refactor: remove incorrect lifetimebounds"
(https://github.com/bitcoin/bitcoin/pull/33870)
πŸš€ fanquake merged a pull request: "ci: Re-enable LINT_CI_SANITY_CHECK_COMMIT_SIG"
(https://github.com/bitcoin/bitcoin/pull/33888)
πŸ’¬ fanquake commented on pull request "depends: Add patch for Windows11Style plugin":
(https://github.com/bitcoin/bitcoin/pull/33906#issuecomment-3559275679)
Concept ACK on applying the patch for now.
πŸ’¬ hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3559295948)
> Concept ACK, but a little more information in the PR description would be helpful, it's hard to parse what expected behaviour is - both from the CI, as well as from developers.

The PR description has been updated.

> Does passing iwyu become mandatory for CI? Are we duplicating CI runs or moving it to a separate iwyu job?

That isn’t the goal of this PR. Other PRs are dedicated to such changes: https://github.com/bitcoin/bitcoin/pull/33725 and https://github.com/bitcoin/bitcoin/pull/337
...
πŸ’¬ hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3559297686)
Rebased.
πŸ’¬ maflcko commented on issue "CI: Improve documentation around replicating CI locally":
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3559299068)
Yeah, I think this can be closed for now, but I'd be excited to see new issues, if there are still questions or hard CI edges. Also, happy to review more pull requests around CI docs.
βœ… fanquake closed an issue: "CI: Improve documentation around replicating CI locally"
(https://github.com/bitcoin/bitcoin/issues/31199)
πŸ’¬ roconnor-blockstream commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3559368666)
I agree. I think my next step would be to PR a change to `AreInputsStandard` to bring P2SH and legacy script policy into alignment.

I'm not quite sure when I'll get to this, so if someone else wants to take a crack at it, I wouldn't be surprised if they can do it better / sooner than me.
πŸ‘ l0rinc approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3487360414)
I love this change, it's really well organized, the commit messages explain each change, there's a lot of context - maybe too much, spread around in multiple PRs.

Concept ACK from me, managed to review until a4fb628d83a344bd866715cee63d7dd9ff03dfff - and peeked into the rest.

My understanding is that this isn't compatible with package relay - what about and cluster mempool, how much will we have to change after it's merged?

Given that this has been in review for a while - I wonder how much we
...
πŸ’¬ l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546177051)
nit:

```suggestion
active_connections += node->IsPrivateBroadcastConn();
```

also note that this isn't exercised currently in the tests (but after this change, it would):
<img width="644" height="185" alt="image" src="https://github.com/user-attachments/assets/81bbe700-5db1-4626-b0fd-ac16905c4593" />
πŸ’¬ l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2545675641)
nit: to avoid [slicing](https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&branch=29415-07650a49f97801701e22af56e6dcba519082eb4b&id=aureleoules_bitcoin&open=AZqcgyl6QjpDlMIZ3R_B&tab=code):
```suggestion
your_addr = addr.IsRoutable() && !IsProxy(addr) && addr.IsAddrV1Compatible() ? CService{addr} : CService{};
```
πŸ’¬ l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546085333)
is this used anywhere? https://corecheck.dev/bitcoin/bitcoin/pulls/29415 claims it's uncovered new code
πŸ’¬ l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2545798247)
I started commenting here, which lead me to read the related comments about this line - there's a lot of history here.
I'm still not sure I understand why this is an identifiable constant (which limits the anonymity set to private broadcasters) and not a randomized set of our currently available peer versions or other possible values (as also suggested in https://github.com/bitcoin/bitcoin/pull/27509#issuecomment-1517904089, which would also cast doubt on the origin of non-private transactions,
...
πŸ’¬ l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2545669887)
nit: as https://corecheck.dev/bitcoin/bitcoin/pulls/29415 also [mentioned](https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&branch=29415-07650a49f97801701e22af56e6dcba519082eb4b&id=aureleoules_bitcoin&open=AZqcgyrEQjpDlMIZ3R_G):
> Define operator<=> and remove operators <, <=, >, >= for operand types "Priority" and "Priority".

maybe something like:
```patch
diff --git a/src/private_broadcast.cpp b/src/private_broadcast.cpp
index 487b071ac9..5c23bfa830 100644
--- a/src/
...
πŸ’¬ l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2546155699)
Wasn't sure whether the range of 0-5 minutes implied that the max was 14 minutes (i.e. whether the millisecond type was taken into consideration), so I ran it a few times and the plots indicate it's correct:

<img width="1008" height="618" alt="Image" src="https://github.com/user-attachments/assets/526e142c-0b77-41ab-ab3d-306a132cf7e1" />