Bitcoin Core Github
43 subscribers
122K links
Download Telegram
πŸ€” janb84 reviewed a pull request: "test: Retry download in get_previous_releases.py"
(https://github.com/bitcoin/bitcoin/pull/33915#pullrequestreview-3491910763)
ACK fad06f3bb436a97683e8248bfda1bd0d9998c899

PR adds one retry to download a prev. release.

The single retry is somewhat crude solution but a good first step to solve the intermitted download issues, good KISS sollution. If this isn't enough we can always fallback on more elegant solutions, but this also increases the complexity.
πŸ€” hebasto reviewed a pull request: "Fix bitcoin-qt visual glitches on Wayland"
(https://github.com/bitcoin-core/gui/pull/904#pullrequestreview-3492565990)
Tested 095f920629207b5ec4c50de7b454dfced0eafefb, it breaks UX.

For example, on Fedora 43 with Gnome 49 and Wayland, follow these steps:
1. Run `bitcoin-qt`.
2. Hide the main window using the "Hide" command in context menu.
3. Click on "Receive" in the system tray icon menu.

On the master branch, the main window reappears.

With this PR, however, the main window remains hidden.
πŸ€” fanquake requested changes to a pull request: "doc: Document compiler configuration for native depends packages"
(https://github.com/bitcoin/bitcoin/pull/33902#pullrequestreview-3492766868)
See https://github.com/bitcoin/bitcoin/pull/33902#discussion_r2549756641.
πŸ€” l0rinc reviewed a pull request: "coins: use number of dirty cache entries in flush warnings/logs"
(https://github.com/bitcoin/bitcoin/pull/33512#pullrequestreview-3492784897)
Thanks for the comments @optout21, since I wasn't invalidating any ACKs, I have applied your suggestions (with rebase) - let me know if this is what you meant.

> keeping a count like the dirty count is very efficient, but prone to going out of sync

yeah, we have a few of these - but the sanitizers and sanity checks and fuzzers usually reveal the problems (sanitizers break on underflow and the sanity checks recalculate everything during testing).
πŸ‘ maflcko approved a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3492941007)
lgtm
πŸ€” hodlinator reviewed a pull request: "build: Embedded ASMap [3/3]: Build binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-3492585142)
### Concept ACK 1dcd2e8a983406d12009e41bac5dbaeb12f330be

I do understand the distaste at adding 1MB+ binary blobs into a reproducible build. From that perspective, it would be cleaner to include the ASMap blob as a separate file included in our Bitcoin Core releases.

But having a separate file opens up the can of worms of where to install it on various operating systems and distributions, how to encourage package maintainers to include and ensure it ends up in the right locations, and making B
...
πŸ‘ maflcko approved a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493037953)
lgtm. makes sense to just mention the stuff that is known to work and remove the stuff that is known to not work
πŸ€” fanquake requested changes to a pull request: "ci: Add Windows + UCRT jobs for cross-compiling and native testing"
(https://github.com/bitcoin/bitcoin/pull/33764#pullrequestreview-3493060075)
https://github.com/bitcoin/bitcoin/pull/33764#discussion_r2549961718
πŸ€” l0rinc requested changes to a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493026362)
Concept ACK, but if we claim best-effort, we should cover the M series as well with latest AppleClang and latest LLVM.
πŸ€” hodlinator reviewed a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3492740506)
Concept ACK e14650967dc95da5c10e0d6183b6eac3e8243fe5

#32541 contained some useful tricks that seem to be useful to shrink other index implementations. Thanks for letting it go in favor of this less invasive change though!
πŸ‘ ismaelsadeeq approved a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493432014)
ACK 6e37b3b4861733ca97ec5d27d0bf52d187b6a2c9
πŸ€” TumaBitcoiner reviewed a pull request: "wallet/refactor: change PSBTError to PSBTResult and remove std::optional<common::PSBTResult> and return common::PSBTResult"
(https://github.com/bitcoin/bitcoin/pull/32958#pullrequestreview-3493400959)
I made some suggestions to keep consistency between naming and files. Sometimes `result` was used, other times `err`.
πŸ€” ismaelsadeeq reviewed a pull request: "mining: add getMemoryLoad() and track template non-mempool memory footprint"
(https://github.com/bitcoin/bitcoin/pull/33922#pullrequestreview-3493499544)
Concept ACK

I think it would be better if we have internal memory management for the mining interface IPC, since we hold on to the block templates.

I would suggest the following approach:

- Add memory budget for the mining interface.

- Introduce a tracking list of recently built block templates and total memory usahe.

- Add templates to the list and increment the memory usage after every `createnewblock` or `waitnext` return.

- Whenever the memory budget is exhausted, we should release tem
...
πŸ€” brunoerg reviewed a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3493836797)
Concept ACK
πŸ‘ darosior approved a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3494282906)
ACK 6e37b3b4861733ca97ec5d27d0bf52d187b6a2c9

nit: i think it would be good to have a sentence that fuzzing support is only maintained for Linux platforms as the first line of the document, so readers know what to expect. It's fine if it's also repeated in the "MacOS notes" section.
πŸ€” hodlinator reviewed a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3493993983)
Concept ACK de6a28396b8bf7a3fd1124418b4cd5beba1906d6

`ComputeMerkleRoot(std::vector<uint256> hashes, ...)` takes a vector by value, but luckily call sites already move into it, and since the underlying array/memory is transferred on move, the capacity "transfers as well" - neat!

---

> Sorry to be this guy again, but it seems to me unreasonable to touch such critical consensus code for such marginal benefits. Unless observable benefits can be shown, ...

Thanks for being that guy!

Going by ht
...
πŸ‘ benthecarman approved a pull request: "Revert "gui, qt: brintToFront workaround for Wayland""
(https://github.com/bitcoin-core/gui/pull/914#pullrequestreview-3496739218)
fixes for me!
πŸ€” l0rinc reviewed a pull request: "merkle: pre‑reserve leaves to prevent reallocs with odd vtx count"
(https://github.com/bitcoin/bitcoin/pull/32497#pullrequestreview-3496417072)
Latest push:
* rebases against latest master;
* reverts code back to simply change `leaves.resize(block.vtx.size())` constructs before calling `ComputeMerkleRoot` to `.reserve((block.vtx.size() + 1) & ~1ULL)`;
** added godbolt reproducer for each important combination of compiler and architecture
* Separates the test changes to a separate commit explaining the motivation in the commit message
* updated commit messages and PR descriptions.
πŸ€” hebasto reviewed a pull request: "contrib: Remove brittle, confusing and redundant UTF8 encoding from Python IO"
(https://github.com/bitcoin/bitcoin/pull/33702#pullrequestreview-3496784212)
I've tested fa2fd0ba1fbd4085df24a2c5472636957db28521 on a Windows 11 laptop using VS 2022 17.14.21.

Functional tests work as expected.
πŸ€” sipa reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3496807348)
ACK 17cf9ff7efdbab07644fc2f9017fcac1b0757c38