💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121686616)
Done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121686616)
Done.
💬 sipa commented on pull request "p2p: improve TxOrphanage denial of service bounds":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2121695775)
What do you think about using the "exposed `std::unique_ptr<>`" pattern (don't know if it has a name) as opposed to full-blown pimpl.
So like `TxGraph` / `TxGraphImpl`, I'm suggesting there would be 2 files (no `_impl.h`), with a `TxOrphanage` abstract class with virtual member functions, and an implementation defined only inside the `.cpp` file, and a factory function `std::unique_ptr<TxOrphanage> MakeTxOrphanage()` that returns a newly-constructed `TxOrphanageImpl`. It avoids the boilerplat
...
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2121695775)
What do you think about using the "exposed `std::unique_ptr<>`" pattern (don't know if it has a name) as opposed to full-blown pimpl.
So like `TxGraph` / `TxGraphImpl`, I'm suggesting there would be 2 files (no `_impl.h`), with a `TxOrphanage` abstract class with virtual member functions, and an implementation defined only inside the `.cpp` file, and a factory function `std::unique_ptr<TxOrphanage> MakeTxOrphanage()` that returns a newly-constructed `TxOrphanageImpl`. It avoids the boilerplat
...
📝 hebasto converted_to_draft a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308)
This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.
(https://github.com/bitcoin/bitcoin/pull/31308)
This PR is the first step towards treating IWYU warnings as errors. At this stage, it applies only to the `crypto` and `index` directories.
👋 hebasto's pull request is ready for review: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308)
(https://github.com/bitcoin/bitcoin/pull/31308)
🤔 jonatack reviewed a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2889257494)
re-ACK 582d9e3dbdf5b64272b65844d679942c5aca643f per `git range-diff 1c66023 c97d496 582d9e3`
changes since my last review https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2446770519 have been rebases and adding commit cf0a49723bbbbb22d47ccb8f0e030a51e6757609
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2889257494)
re-ACK 582d9e3dbdf5b64272b65844d679942c5aca643f per `git range-diff 1c66023 c97d496 582d9e3`
changes since my last review https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2446770519 have been rebases and adding commit cf0a49723bbbbb22d47ccb8f0e030a51e6757609
💬 fanquake commented on pull request "build: add a depends dependency provider":
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2931604386)
https://cirrus-ci.com/task/5352790488252416?logs=ci#L1675:
```bash
[09:31:48.288] Extracting freetype...
[09:31:48.316] /ci_container_base/depends/sources/freetype-2.11.0.tar.xz: OK
[09:31:48.562] Preprocessing freetype...
[09:31:48.575] Configuring freetype...
[09:31:48.656] CMake Error at CMakeLists.txt:100 (cmake_minimum_required):
[09:31:48.656] Compatibility with CMake < 3.5 has been removed from CMake.
[09:31:48.656]
[09:31:48.656] Update the VERSION argument <min> value. Or
...
(https://github.com/bitcoin/bitcoin/pull/32595#issuecomment-2931604386)
https://cirrus-ci.com/task/5352790488252416?logs=ci#L1675:
```bash
[09:31:48.288] Extracting freetype...
[09:31:48.316] /ci_container_base/depends/sources/freetype-2.11.0.tar.xz: OK
[09:31:48.562] Preprocessing freetype...
[09:31:48.575] Configuring freetype...
[09:31:48.656] CMake Error at CMakeLists.txt:100 (cmake_minimum_required):
[09:31:48.656] Compatibility with CMake < 3.5 has been removed from CMake.
[09:31:48.656]
[09:31:48.656] Update the VERSION argument <min> value. Or
...
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121717145)
Thanks! Reworked.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121717145)
Thanks! Reworked.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121717600)
Sure! Fixed.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121717600)
Sure! Fixed.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121718094)
Thanks! Fixed.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121718094)
Thanks! Fixed.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121718259)
Thanks! Fixed.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121718259)
Thanks! Fixed.
💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121718625)
Thanks! Reworked.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121718625)
Thanks! Reworked.
💬 darosior commented on pull request "policy: make pathological transactions packed with legacy sigops non-standard":
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121720547)
Done.
(https://github.com/bitcoin/bitcoin/pull/32521#discussion_r2121720547)
Done.
🤔 jonatack reviewed a pull request: "fuzz: set the output argument of FuzzedSock::Accept()"
(https://github.com/bitcoin/bitcoin/pull/31316#pullrequestreview-2889292340)
utACK 83199523c90591d57cd5046212c878a4d54d621d
This commit was also pulled into #28584 as https://github.com/bitcoin/bitcoin/pull/28584/commits/cf0a49723bbbbb22d47ccb8f0e030a51e6757609 and is identical at the time of review.
(https://github.com/bitcoin/bitcoin/pull/31316#pullrequestreview-2889292340)
utACK 83199523c90591d57cd5046212c878a4d54d621d
This commit was also pulled into #28584 as https://github.com/bitcoin/bitcoin/pull/28584/commits/cf0a49723bbbbb22d47ccb8f0e030a51e6757609 and is identical at the time of review.
💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121738729)
Ok, so no an issue if we actually land the patch (and remove the related code in the CI).
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121738729)
Ok, so no an issue if we actually land the patch (and remove the related code in the CI).
🤔 jonatack reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2889310631)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2889310631)
Concept ACK
💬 jonatack commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121737144)
Last I looked, enforcing this across the codebase may require large changes in some areas to untangle everything.
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121737144)
Last I looked, enforcing this across the codebase may require large changes in some areas to untangle everything.
📝 fanquake converted_to_draft a pull request: "uint256 cxx-20 constexpr patch"
(https://github.com/bitcoin/bitcoin/pull/32663)
(https://github.com/bitcoin/bitcoin/pull/32663)
💬 instagibbs commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2121791082)
@mzumsande pushed as a follow-up commit. lmk what you think.
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2121791082)
@mzumsande pushed as a follow-up commit. lmk what you think.
💬 bigspider commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2931818779)
> @bigspider what version of the Ledger test app is required? And can it just sign (or provide the nonce) the PSBT (via HWI) and register the policy on first use? Or do you have to first explicitly register the policy like [Moosig](https://github.com/LedgerHQ/moosig/blob/master/moosig.py) does?
@Sjors, musig2 is supported from version 2.4.0 of the Bitcoin app, which at this time is the latest version. Note that the firmware OS must be up to date, or you would only find older versions of the a
...
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2931818779)
> @bigspider what version of the Ledger test app is required? And can it just sign (or provide the nonce) the PSBT (via HWI) and register the policy on first use? Or do you have to first explicitly register the policy like [Moosig](https://github.com/LedgerHQ/moosig/blob/master/moosig.py) does?
@Sjors, musig2 is supported from version 2.4.0 of the Bitcoin app, which at this time is the latest version. Note that the firmware OS must be up to date, or you would only find older versions of the a
...
💬 jonatack commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2931826727)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2931826727)
Concept ACK