Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1955404279)
In commit "ci: build multiprocess for all depends jobs" (d937f3abdccdfdf1008dceb7a4235b1cbab9a3ae)

This is a documentation change. not really a CI change, so ideally I think it wouldn't be part of this commit.

I think the most logical place to move this change would be commit 63dffe1795d9e7b1d3307e0982dbbea2f1563f0f in followup PR #31802, because that's the commit which adds the corresponding depends setting (`NO_MULTIPROCESS ?= $(if $(findstring mingw32,$(HOST))$(findstring openbsd,$(HOST
...
👍 tdb3 approved a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2616521405)
re ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53
💬 eval-exec commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955435179)
Agree, I will append a commit to fix this.
💬 eval-exec commented on pull request "random: Check `GetRNDRRS` is supported in `InitHardwareRand` to avoid infinite loop":
(https://github.com/bitcoin/bitcoin/pull/31826#discussion_r1955435465)
Agreed, I'll refactor to avoid duplication with `GetRNDRRS()`.
⚠️ luke-jr opened an issue: "Wallet/transaction notifications: Only a single notification is displayed"
(https://github.com/bitcoin-core/gui/issues/853)
There are actually multiple issues here, at least:
1. `WalletView::processNewTransaction` only looks at the first of a batch of inserted rows.
2. `QSystemTrayIcon`'s X11 implementation (`QBalloonTip`, internal-only) only allows a single notification, destroying the previous when a new one is shown
3. Showing possibly hundreds of notifications would be terrible UX

It seems like the GUI should queue notifications at the same "instant" (eg, a new block), and if there's multiple send a summary of t
...
💬 goodthebest commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#issuecomment-2658235090)
I think it's relevant thread. Can we mine bitcoin using ```prune``` flag, what is the ideal or minimum required prune value for a pool's node to be mineable?
💬 davidgumberg commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2658300687)
utreACK https://github.com/bitcoin/bitcoin/commit/851085a7f2761584402080b1767328d86cde4d94
💬 davidgumberg commented on pull request "doc: build: Fix instructions for msvc gui builds":
(https://github.com/bitcoin/bitcoin/pull/31851#issuecomment-2658310622)
I considered that, but this solution is much more sane than any of the others I managed to come up with, and the flag has been around for at least 4 years (https://github.com/microsoft/vcpkg/issues/13789#issuecomment-700140607) , and this is only documentation, if there is a better solution, especially one that doesn't require the user to intervene in any way, that would be ideal.
💬 maflcko commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2658378354)
@davidgumberg Looks like you reviewed a stale commit?
💬 maflcko commented on pull request "test: deduplicates p2p_tx_download constants":
(https://github.com/bitcoin/bitcoin/pull/31758#issuecomment-2658397438)
review ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53 🔖

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 0a02e7fdeaca
...
💬 laanwj commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2658440020)
changes between 851085a7f2761584402080b1767328d86cde4d94 and current tip:
https://github.com/bitcoin/bitcoin/compare/851085a7f2761584402080b1767328d86cde4d94..b9d4d5f66a5a35c47e7abc9ec6ef5ab242b3f1e1
💬 maflcko commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658475173)
Looks like the discussion is a bit scattered around the place. No strong opinion, but some notes (or a summary):

* A pull request is already open and I left a comment there to list the stuff I noticed that is using GCC coverage: https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2540826789
* Coverage on debuginfo-level (post-optimization) surely is different than coverage on source-code-level, but I wouldn't call it "broken".
* All of this is mostly a test-only dev-only issue, so I don'
...
📝 jirijakes opened a pull request: "doc: Fix description of byte order of hashes in ZMQ documentation"
(https://github.com/bitcoin/bitcoin/pull/31862)
ZMQ produces transaction and block hashes in the same format as RPC, that is with bytes in a reversed order from the output of hashing function. Previously, the ZMQ documentation incorrectly informed that the two are in different formats, and this changes fixes this.

Additionally, as the terms 'Big Endian' and 'Little Endian' are misleading in terms of order of bytes in an arbitrary sequence of bytes, this change also removes this notion and uses the term 'reversed byte order' instead.

Fix
...
💬 davidgumberg commented on pull request "net: Use GetAdaptersAddresses to get local addresses on Windows":
(https://github.com/bitcoin/bitcoin/pull/31014#issuecomment-2658490999)
> @davidgumberg Looks like you reviewed a stale commit?

Oops.

utreACK https://github.com/bitcoin/bitcoin/commit/b9d4d5f66a5a35c47e7abc9ec6ef5ab242b3f1e1

Recent rebase makes a comment change, and removes an unnecessary loop, addressing reviewer feedback.
💬 jirijakes commented on issue "doc/zmq: Note about endianness does not match reality":
(https://github.com/bitcoin/bitcoin/issues/31856#issuecomment-2658493762)
> current ZMQ documentation is mostly wrong describe RPC interface as using big endian format

I don't know where the notion of Big/Little Endian of hashes comes from but I have seen the RPC-style (reversed by order) to be mostly described as Big Endian, for example [learnmebitcoin](https://learnmeabitcoin.com/technical/general/byte-order/#reverse-byte-order) uses it. Anyway, it is apparently very misleading.

> Best way to fix this would be to avoid using terms little endian and big endian at a
...
👍 hebasto approved a pull request: "doc: build: Fix instructions for msvc gui builds"
(https://github.com/bitcoin/bitcoin/pull/31851#pullrequestreview-2616969253)
ACK c3fa043ae560289759b6f836ac62736d97ba91bf.
💬 maflcko commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2658504053)
> so much is different with the `std::chrono` implementation that anything similar is unlikely to re-occur.

To give some more context, if this exact issue were to re-appear, it would now be caught by other tests (maybe).

For testing, I took the constant offset from https://github.com/bitcoin/bitcoin/issues/3494#issuecomment-31849134: `std::chrono::seconds{0x0000ffc4'00000000}` and applied it to the system clock as offset. Given that the system clock is normally in nanosecond precision, thi
...
💬 maflcko commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2658549377)
> I suggest treating the broken GCC-based coverage as a 29.0 blocker, as it seems it's been effectively unusable anyway.

I don't see why this should be a 29.0 blocker at all, or why GCC is unusable, given that it is used, for example, see the previous comment above: https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2540826789

Let's continue discussion in the thread https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2658475173?
💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r1955758861)
AFAIU the second flush is the original one, while the second flush was introduced in https://github.com/bitcoin/bitcoin/commit/3192975f1d177aa9f0bbd823c6387cfbfa943610 to help cleanly process all remaining callbacks. Maybe it could have removed the second flush in that commit?

That said I think it is good practice to be defensive here and keep both.
👍 TheCharlatan approved a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2617102083)
Re-ACK 9b033bebb18dfd609c02736292f37cc6589fcc8d