💬 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 ✅

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2426186301)
Done thanks

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2426187161)
This test is redundant removed.

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2426187649)
This test is also redundant removed.

💬 ismaelsadeeq commented on pull request "Policy: Report reason inputs are non standard":
(https://github.com/bitcoin/bitcoin/pull/29060#discussion_r2426187998)
fixed