Bitcoin Core Github
39 subscribers
143K links
Download Telegram
πŸ’¬ kevkevinpal commented on pull request "util: Return uint64_t from _MiB operator":
(https://github.com/bitcoin/bitcoin/pull/35097#issuecomment-4271227664)
Concept ACK

It makes sense to return `uint64_t` instead of `size_t` and this code change is pretty minimal and scoped to just a few files.
πŸ‘ ryanofsky approved a pull request: "logging: better use of log::Entry internally"
(https://github.com/bitcoin/bitcoin/pull/34865#pullrequestreview-4132255215)
Code review ACK c5ec2d53130e80ce19daa98f15a688fbc6b28a3f. Just suggested changes since last review: adding (void) std::move, making SourceLocation constructor explicit, dropping BufferedLog struct. Overall PR is a nice code simplification, and I don't think it has downsides other than buffered logs now containing [ignored](https://github.com/bitcoin/bitcoin/pull/34865#discussion_r3054379230) `should_ratelimit` fields, which can be fixed in a followup by separating log entries from log options.
πŸ€” optout21 reviewed a pull request: "net: deduplicate private broadcast state and snapshot types"
(https://github.com/bitcoin/bitcoin/pull/35016#pullrequestreview-4132276136)
Concept ACK

A relatively straightforward merging of duplicated structs, done in two stages to minimize diffs. An existing unit test has been also extended with some extra checks.
I have verified by reimplementing the moves and updates.
Left some remarks, none major.
πŸ’¬ optout21 commented on pull request "net: deduplicate private broadcast state and snapshot types":
(https://github.com/bitcoin/bitcoin/pull/35016#discussion_r3103412490)
> [4a89e31](https://github.com/bitcoin/bitcoin/pull/35016/changes/4a89e31da7ffd9100f6522745db2a55bb0f5f67e) _net: drop TxSendStatus from private broadcast snapshot_:

I find it confusing such a large block added in a commit, then removed in the next. I think it would be better to drop it.
πŸ’¬ optout21 commented on pull request "net: deduplicate private broadcast state and snapshot types":
(https://github.com/bitcoin/bitcoin/pull/35016#discussion_r3103418951)
> [4a89e31](https://github.com/bitcoin/bitcoin/pull/35016/changes/4a89e31da7ffd9100f6522745db2a55bb0f5f67e) _net: drop TxSendStatus from private broadcast snapshot_:

These time handling changes are not strictly related to the scope of the PR. I suggest isolating them in a separate commit, best the last commit.
πŸ’¬ optout21 commented on pull request "net: deduplicate private broadcast state and snapshot types":
(https://github.com/bitcoin/bitcoin/pull/35016#discussion_r3103436409)
> [4a89e31](https://github.com/bitcoin/bitcoin/pull/35016/changes/4a89e31da7ffd9100f6522745db2a55bb0f5f67e) _net: drop TxSendStatus from private broadcast snapshot_:

With this change from using `TxSendStatus` to `TxBroadcastInfo`, an additional 3rd `CTransactionRef tx` field is included. I feel this side effect is important enough to be explained in the commit message.

Also, please fix the formatting of the commit message (`\n` characters show up instead of newline).
πŸ€” mzumsande reviewed a pull request: "validation: prevent FindMostWorkChain from causing UB"
(https://github.com/bitcoin/bitcoin/pull/35070#pullrequestreview-4133235432)
> While reviewing, noticed another source of duplicate insertion and potential UB though:

> This can be fixed by guarding the insertion in LoadBlockIndex with BLOCK_HAVE_DATA:

I agree.
The `LoadBlockIndex` issue is also described in issue #35050, which suggests the same fix.
πŸ“ kevkevinpal opened a pull request: "ci, iwyu: add compat to the list of FILES_WITH_ENFORCED_IWYU"
(https://github.com/bitcoin/bitcoin/pull/35102)
## Description
Motivated by https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3466897433 this continues the effort to enforce iwyu warnings.

This is only adding the `compat` dir to the CI script since it seems to already pass the iwyu rules.

Including `compat` into the CI will keep that part of the codebase hygienic

---

I can close this if it isn't needed or necessary.
πŸ’¬ frankomosh commented on pull request "fuzz: add p2p_private_broadcast harness":
(https://github.com/bitcoin/bitcoin/pull/35090#discussion_r3104498239)
Seemingly so, I can see that it also calls `peerman→SendMessages()`, so would probably also need the fabf8d1 fix. Would it be appropriate to add it here as a second commit or just open a separate PR? Vasild has proposed deduplicating some features here https://github.com/bitcoin/bitcoin/pull/35090#discussion_r3093704068, Do you think that would help?
πŸ“ BrandonOdiwuor opened a pull request: "ci, iwyu: Fix warnings in `src/compat/` & `src/support/` and treat them as error"
(https://github.com/bitcoin/bitcoin/pull/35103)
This PR [continues](https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3466897433) the ongoing effort to enforce IWYU warnings.(see https://github.com/bitcoin/bitcoin/pull/35011#issuecomment-4214396321)

See [Developer Notes](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#using-iwyu).
βœ… maflcko closed a pull request: "ci, iwyu: Fix warnings in `src/compat/` & `src/support/` and treat them as error"
(https://github.com/bitcoin/bitcoin/pull/35103)
πŸ’¬ maflcko commented on pull request "ci, iwyu: Fix warnings in `src/compat/` & `src/support/` and treat them as error":
(https://github.com/bitcoin/bitcoin/pull/35103#issuecomment-4273039114)
Please do not spam duplicate pull requests. This is now the third or forth one, see literally the prior pull request https://github.com/bitcoin/bitcoin/pull/35102.

Also, you have yourself a pull request on this topic already. Instead of ignoring review feedback on your own pull request, and creating even more, it would be better to just have a single pull request on this topic and actually work on it by replying to the feedback or addressing it, and then waiting for it to be merged.

Thx,
...
βœ… maflcko closed a pull request: "ci, iwyu: add compat to the list of FILES_WITH_ENFORCED_IWYU"
(https://github.com/bitcoin/bitcoin/pull/35102)
πŸ’¬ maflcko commented on pull request "ci, iwyu: add compat to the list of FILES_WITH_ENFORCED_IWYU":
(https://github.com/bitcoin/bitcoin/pull/35102#issuecomment-4273042654)
Thx, but I don't think it is useful to create conflicting pull requests with already open ones, when the diff is a single line change and that single line conflicts.

It would be better to instead review the existing pull request.

I am going to close this for now. You can pick this up again when the other one is closed or merged, and when this is still relevant after it is closed/merged.
πŸ’¬ batmanbytes commented on pull request "consensus: soft fork on testnet4 that fixes the min difficulty blocks exploit":
(https://github.com/bitcoin/bitcoin/pull/35081#issuecomment-4273180477)
In https://github.com/bitcoin/bitcoin/pull/35081/commits/6b744183bd08abaead455004810ca81c57e40af5 I removed the new timestamp rule for difficulty-adjustment blocks, since it is mandatory for the starting block of an epoch to use real difficulty and not min difficulty.

This means that if the average block time of the 75th epoch is 60 minutes, then the avg block time of the 76th will be 30 minutes, the next 15 minutes and we will then stabilize in 10. Cleaner, and less disruptive.

I will
...
πŸ’¬ fjahr commented on pull request "test: interface_http follow-ups":
(https://github.com/bitcoin/bitcoin/pull/35086#issuecomment-4273296020)
post-merge ACK f49a2afd94c3b8a5c06d7b77155d0596300bdc2e
⚠️ hebasto opened an issue: "release: Checksum mismatch in `guix-codesign` log"
(https://github.com/bitcoin/bitcoin/issues/35104)
When codesigning Windows binaries for 31.0, `guix-codesign` logs multiple checksum mismatches:
```
$ env HOSTS=x86_64-w64-mingw32 ./contrib/guix/guix-codesign
Checking that we can connect to the guix-daemon...

Hint: If this hangs, you may want to try turning your guix-daemon off and on
again.

INFO: Codesigning 31.0 for platform triple x86_64-w64-mingw32:
...using reference timestamp: 1776286524
...from worktree directory: '/home/hebasto/dev/bitcoin'
...bind-mounted
...
πŸ’¬ hebasto commented on issue "release: Checksum mismatch in `guix-codesign` log":
(https://github.com/bitcoin/bitcoin/issues/35104#issuecomment-4273445595)
cc @achow101
πŸ’¬ takeshikurosawaa commented on pull request "net: deduplicate private broadcast state and snapshot types":
(https://github.com/bitcoin/bitcoin/pull/35016#issuecomment-4273449631)
Thanks for the review. I've cleaned up the history to remove the transient snapshot_structural_invariant add/remove step, split the NodeClockContext changes into a final test-only commit, and clarified the duplicated CTransactionRef storage tradeoff in the main commit message while fixing its formatting.
πŸ’¬ fjahr commented on pull request "tor: limit torcontrol line size that is processed to prevent OOM":
(https://github.com/bitcoin/bitcoin/pull/35087#issuecomment-4273451752)
ACK 9fe5896a446d5a1acb5834561ffbca841f8a970a