Bitcoin Core Github
44 subscribers
121K links
Download Telegram
👍 rkrux approved a pull request: "netinfo: add peer services column and outbound-only option"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2398918550)
tACK 87532fe55856efc063cf81244800da37a015ba75

Successful make and functional tests. I used the `-netinfo` command with various verbosities and the `outonly` option. Also checked for verbosities greater than 4.

Asked a question for clarity.
💬 rkrux commented on pull request "netinfo: add peer services column and outbound-only option":
(https://github.com/bitcoin/bitcoin/pull/30930#discussion_r1818964134)
Can `NODE_NONE` ever be a value here?

https://github.com/bitcoin/bitcoin/blob/master/src/protocol.h#L312
👍 tdb3 approved a pull request: "test: fix intermittent failure in p2p_seednode.py, don't connect to random IPs"
(https://github.com/bitcoin/bitcoin/pull/31142#pullrequestreview-2398927299)
cr and light test ACK 6c9fe7b73ea1572b8b56c716ab13d9866f91c6e9

Nice addition of the non-working proxy.
💬 hebasto commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#issuecomment-2441444414)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.
📝 fanquake opened a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172)
Follows up to https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1803165670.

We definitely cannot claim that Bitcoin Core is "supported and extensively tested on" on Windows 7.

Note that #30997 is also increasing the minimum required Windows version (for the GUI) to 10.
💬 fanquake commented on pull request "build: Bump minimum supported macOS to 13.0":
(https://github.com/bitcoin/bitcoin/pull/31048#discussion_r1818970819)
Yea, and this is already the case with the current binary. See #31172.
💬 hebasto commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2441463955)
My Guix build:
```
aarch64
fba7e542bc6a35c65a179f9c8cd3ae2b0f8b12cd5895fa255a2d344d576d0215 guix-build-915640e191b6/output/aarch64-linux-gnu/SHA256SUMS.part
c9c1d47120de9a45aa5da1c17cc8f4860f2268bfe413d38e1b6f67ea96bd4f09 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu-debug.tar.gz
c1775debe30e5a28d981720582c5e6f36ea33939169a429edc48c4bb8ebc3688 guix-build-915640e191b6/output/aarch64-linux-gnu/bitcoin-915640e191b6-aarch64-linux-gnu.tar.gz
870a9d4f
...
💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2441473829)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2441487157)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31130.
🤔 naumenkogs reviewed a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2398752467)
ACK 0f4bc635854597e15ea6968767fc4e5cf5bdd790
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818860807)
now that i realize it's a very internal thing to the download manager, i understand why this name was chosen. resolve pls.
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818926518)
nit

f497414ce76a4cf44fa669e3665746cc17710fc6 still has `Assume` removed
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818882978)
042a97ce7fc672021cdb1dee62a550ef19c208fb

code-wise i think this better belongs around `MakeAndPushMessage` (what if the caller decides to postpone the actual sending), but no big deal.
💬 naumenkogs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1818893833)
Okay, so

` /** New inv has been received. May be added as a candidate to txrequest. */` is inaccurate, right? There is no new INV received in `p2p_inv=false` case, but rather it's called from orphan consideration after processing a TX?
👍 TheCharlatan approved a pull request: "build: Rename `PACKAGE_*` variables to `CLIENT_*`"
(https://github.com/bitcoin/bitcoin/pull/31042#pullrequestreview-2399049784)
Re-ACK 70713303b6399e0627c1960da4fbab48f9cf71e7
🤔 stickies-v reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2393162554)
Note: `doc/bips.md` needs updating, "opt-in" is no longer accurate: https://github.com/bitcoin/bitcoin/blob/1c7ca6e64de9b47b2295c81cb0fedd432ffaf001/doc/bips.md?plain=1#L37
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819051835)
Keeping this section here feels unnecessarily confusing to me. Since it's no longer relevant, I'd just remove the entire paragraph, and potentially add a line in the `## History` section?
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1818979310)
nit: suggestion to provide a bit more context. Ideally, at least the PR number is added.
```suggestion
Starting with v28.0, the `mempoolfullrbf` startup option was set to
default to `1`. With widespread adoption of this policy, users no longer
benefit from disabling it, so the option has been removed, making full
replace-by-fee the standard behavior. (#30592)
```
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1815326806)
nit: would just drop this line (+newline entirely), already covered in `self.log.info('A transaction that replaces a mempool transaction')`
💬 stickies-v commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1819055432)
(note: this did not get fixed - it's a trivial re-ack, would be ideal to get this in this PR - it's very confusing when reading the code)