Bitcoin Core Github
44 subscribers
121K links
Download Telegram
πŸ‘ stickies-v approved a pull request: "[28.x] 28.3rc2"
(https://github.com/bitcoin/bitcoin/pull/33557#pullrequestreview-3331061515)
ACK 8e4651bb33b8f97940bf2ad6a2f97932198f6765 modulo 1 link update required (see comment)

All backports clean except:
- 9c911f7e2dcb6dc26b5824bdae2d389cc931607e backported from abf4a6eeaee116917dafd56eb9caee03e13048d2: merge conflict due to qt being bumped to `5.15.16` on `master` and still at `5.15.14` on `28.x`.

Getting identical manpages, and release notes look complete.
πŸ’¬ stickies-v commented on pull request "[28.x] 28.3rc2":
(https://github.com/bitcoin/bitcoin/pull/33557#discussion_r2425963692)
Download link on l3 needs to be updated:

<details>
<summary>git diff on 8e4651bb33</summary>

```diff
diff --git a/doc/release-notes.md b/doc/release-notes.md
index b8b90475c8..f446c14cc5 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -1,6 +1,6 @@
Bitcoin Core version 28.3rc2 is now available from:

- <https://bitcoincore.org/bin/bitcoin-core-28.3/test.rc1/>
+ <https://bitcoincore.org/bin/bitcoin-core-28.3/test.rc2/>

This release includes various bug fixe
...
πŸ‘ stickies-v approved a pull request: "[28.x] 28.3rc2"
(https://github.com/bitcoin/bitcoin/pull/33557#pullrequestreview-3331102990)
re-ACK 44d05b2fb25b0a5f14e7487c792ac25ad5f5c284
πŸ‘ willcl-ark approved a pull request: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33416#pullrequestreview-3331104875)
ACK aee53d60d32ec800357336fc8c8602405f767c34

Backports look good.

> Should also backport #31407

Do you want to respond to @achow101's suggestion to backport this #31407? I guess an argument against doing so is that it's apretty large change, and 27.x is out of maintenance?
πŸ’¬ fanquake commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33416#issuecomment-3397051344)
Yea, I'm not planning on adding any further changes to this PR (which will likely be the last to the `27.x` branch).
πŸ’¬ hebasto commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2426003276)
> Why?

Because this PR is not supposed to change behaviour.

> We should not interfere with `CMAKE_EXPORT_COMPILE_COMMANDS`. If developers set this option globally in their dotfiles because it will give them code completion in their IDE, setting it to `OFF` here will break their setup.

When the decision was made to disable the `EXPORT_COMPILE_COMMANDS` property for subtree targets during the migration from Autotools to CMake, this use case was not considered, but only the β€œtidy” CI job,
...
πŸ€” marcofleon reviewed a pull request: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33416#pullrequestreview-3331161979)
lgtm ACK aee53d60d32ec800357336fc8c8602405f767c34
πŸš€ fanquake merged a pull request: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33416)
πŸ’¬ fanquake commented on pull request "[27.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33416#issuecomment-3397094073)
I've closed the [27.3 milestone](https://github.com/bitcoin/bitcoin/milestone/76), as there's not currently a plan to make a `27.3` release.
πŸ’¬ ismaelsadeeq commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3397095596)
> nit: if it's already in a fees/ subfolder i think naming them estimator.{h,cpp} would be as clear and thrice as short?

Hmm, I wanted it to be specific, thats why I named it block policy estimator and the file being in the fees directory was meant to indicate it is estimating fees.
πŸ’¬ ismaelsadeeq commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#discussion_r2426029167)
will fix nits when retouching.
πŸ’¬ marcofleon commented on pull request "[28.x] 28.3rc2":
(https://github.com/bitcoin/bitcoin/pull/33557#issuecomment-3397098352)
Nice, re ACK 44d05b2fb25b0a5f14e7487c792ac25ad5f5c284
πŸ’¬ TheCharlatan commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2426025067)
I'm a bit confused by this change too. Previously we wrote the reindexing flag before starting to connect blocks (through the call to ABC), but now it seems like we are connecting blocks first, and only then writing the reindexing flag again?
πŸ’¬ hebasto commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2426034017)
As stated in the comment above, this is done intentionally to avoid cluttering the CMake cache with options originating from a subtree, which should never be modified by the user.
πŸ’¬ TheCharlatan commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#issuecomment-3397101094)
Concept ACK
πŸ’¬ hebasto commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#discussion_r2426056415)
I agree that using `block()` would be more appropriate. For now, this code follows the approach used here: https://github.com/bitcoin/bitcoin/blob/563747971be492a8da772fb2f3e45dd5ee05da86/src/CMakeLists.txt#L23-L24 for consistency.
πŸš€ fanquake merged a pull request: "[28.x] 28.3rc2"
(https://github.com/bitcoin/bitcoin/pull/33557)
πŸ’¬ hebasto commented on pull request "Update `minisketch` subtree and switch to its build script":
(https://github.com/bitcoin/bitcoin/pull/32856#issuecomment-3397150568)
Refreshed and rebased. The PR description has been updated.
πŸ’¬ caesrcd commented on pull request "contrib: Add bash completion for new bitcoin command":
(https://github.com/bitcoin/bitcoin/pull/33385#issuecomment-3397213980)
@prusnak It will complete the arguments of `bitcoin` itself and the subarguments of commands that already have their own completion.

To cover `bitcoin test`, a completion script for the `test_bitcoin` command would need to be created.
πŸ€” janb84 reviewed a pull request: "refactor: Construct g_verify_flag_names on first use"
(https://github.com/bitcoin/bitcoin/pull/33600#pullrequestreview-3331335695)
lgtm ACK faa9d10c84bc6b465cbca266468990cc716b4300

PR refactors `g_verify_flag_names` init to use the "Construct on first use" Idiom to prevent a "static initialization order fiasco" error. This currently gives some false positives, imho a valid change to remove the false positives and to prevent a "static initialization order fiasco" type of error in the future.

- code review βœ…
- build en tests run βœ