Bitcoin Core Github
44 subscribers
120K links
Download Telegram
👍 laanwj approved a pull request: "contrib: remove bdb exception from FORTIFY check"
(https://github.com/bitcoin/bitcoin/pull/32448#pullrequestreview-2825462550)
Code review ACK f9dfe8d5e0d3f628659702ab781b7919505c829f
🤔 ismaelsadeeq reviewed a pull request: "doc: warn that CheckBlock() underestimates sigops"
(https://github.com/bitcoin/bitcoin/pull/31624#pullrequestreview-2825308443)
ACK 0ac19a98f329fe8952f394509a7a29775ab805b0 with a nit and another suggestion


The size limit check above the sigops check also underestimates the block size because it does not include the witness.
💬 ismaelsadeeq commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2079816983)
nit: maybe be specific.
```suggestion
// This underestimates the number of sigops, because unlike ConnectBlock it does not count the witness sigops and p2sh sigops:

```
💬 ismaelsadeeq commented on pull request "doc: warn that CheckBlock() underestimates sigops":
(https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2079904329)
I think it's okay to have this check here to catch those violations early.
🚀 ryanofsky merged a pull request: "cmake: Respect user-provided configuration-specific flags"
(https://github.com/bitcoin/bitcoin/pull/32356)
💬 ismaelsadeeq commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-2863404137)
Friendly ping @glozow @willcl-ark @instagibbs
💬 laanwj commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#issuecomment-2863424559)
> When someone builds from a tarball that is more than a year old, this will use the current year instead of the year of the release. Is that a valid use case?

Not only with tarballs, it's true for any non-guix build, also from an old git commit.
💬 instagibbs commented on pull request "fees: rpc: `estimatesmartfee` now returns a fee rate estimate during low network activity":
(https://github.com/bitcoin/bitcoin/pull/32395#issuecomment-2863430236)
Is this something we need to support? In a sense the current mechanism is honestly saying it doesn't have enough data. If you don't actually know what level of fees are required to get in a block, you may miss the fact that, f.e., miners are not mining anything 2x higher than your floating minfee.

I think this circles back to what the end goal of this RPC is, A one stop shop for best guess of feerates needed?

I haven't thought about this much in years, so I'd be curious to hear what you th
...
🤔 mzumsande reviewed a pull request: "policy: uncap datacarrier by default"
(https://github.com/bitcoin/bitcoin/pull/32406#pullrequestreview-2825526715)
Concept / Approach ACK, wrote the reason in [the other PR](https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2855558032)
💬 ryanofsky commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#discussion_r2079946510)
In commit "build: let CMake determine the year" (dfeac76b951fc2d28e0832b176f3c8ad595697f1)

Doesn't this make the build nondeterministic?
💬 laanwj commented on pull request "build: let CMake determine the year":
(https://github.com/bitcoin/bitcoin/pull/32445#discussion_r2079949509)
No; SOURCE_DATE_EPOCH, which is set by the guix build, is respected: https://github.com/bitcoin/bitcoin/pull/32445#pullrequestreview-2824986604
💬 hebasto commented on issue "build: cmake --install fails after --target deploy":
(https://github.com/bitcoin/bitcoin/issues/32007#issuecomment-2863466359)
From CMake [docs](https://cmake.org/cmake/help/latest/variable/CMAKE_SKIP_INSTALL_ALL_DEPENDENCY.html):
> By default, the `install` target depends on the `all` target. This has the effect, that when `make install` is invoked or `INSTALL` is built, first the `all` target is built, then the installation starts.

[On the other hand](https://cmake.org/cmake/help/latest/manual/cmake.1.html#install-a-project), the `cmake --install <dir>` command:
> ... may be used _after_ building a project to run ins
...
💬 rstmsn commented on pull request "Remove arbitrary limits on OP_Return (datacarrier) outputs":
(https://github.com/bitcoin/bitcoin/pull/32359#issuecomment-2863484560)
The OP_RETURN `-datacarriersize` limit is 100% effective in cases where miners use it to filter their local mempools & inform template construction, prior to successfully mining a block.

This viable use case would be harmed as a result of this PR. On that basis, firm NACK.
💬 fanquake commented on pull request "cmake: Respect user-provided configuration-specific flags":
(https://github.com/bitcoin/bitcoin/pull/32356#issuecomment-2863491522)
Backported to 29.x in #32292.
💬 fanquake commented on pull request "net, pcp: handle multi-part responses and filter for default route while querying default gateway":
(https://github.com/bitcoin/bitcoin/pull/32159#issuecomment-2863507309)
@laanwj you might be interested in circling back here to review?
👍 hebasto approved a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189)
Approach ACK d8caa10c29321c9732dde3d4ccdcf0413041b33f. I've reviewed the code. Still waiting for my Guix build.
🤔 hebasto reviewed a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825611533)
My Guix build:
```
aarch64
5bde67b2713e02dbc6fcf5aa5b90e21df89f7fd04debb4ba13302a8e6bcabc53 guix-build-d8caa10c2932/output/aarch64-linux-gnu/SHA256SUMS.part
09eeaa166590208ce1cdd5bd9298f4819a8b6014fdaf68f421f4797f5bc2d034 guix-build-d8caa10c2932/output/aarch64-linux-gnu/bitcoin-d8caa10c2932-aarch64-linux-gnu-debug.tar.gz
2ecfed14ba3fd7ee565969023e78b810a432042f719c30dd7962b6e070d712ef guix-build-d8caa10c2932/output/aarch64-linux-gnu/bitcoin-d8caa10c2932-aarch64-linux-gnu.tar.gz
1924ef5f
...
💬 fanquake commented on pull request "build: simplify *ifaddr handling":
(https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079998731)
I've dropped the addition.
👍 hebasto approved a pull request: "build: simplify *ifaddr handling"
(https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825625856)
ACK ab878a7e741073574336c9c4b1d41c6cf80b0d6a. Only addressed my [comment](https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079994126) and rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189).