💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951563218)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951563218)
Fixed.
💬 maflcko commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2652009268)
In any case, I am sure this script is broken or at least stale for years and shouldn't be a blocker for 29.0 (even if it was used, it is a dev-only tool). So I'll be removing the milestone for now.
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2652009268)
In any case, I am sure this script is broken or at least stale for years and shouldn't be a blocker for 29.0 (even if it was used, it is a dev-only tool). So I'll be removing the milestone for now.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951565256)
Creating a reference doesn't cost anything (in simple cases); it's just introducing a new name for an existing object. It's true that it's kind of pointless at this stage, but it'll be used in "group per-graph data in ClusterSet", and in "add staging support".
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951565256)
Creating a reference doesn't cost anything (in simple cases); it's just introducing a new name for an existing object. It's true that it's kind of pointless at this stage, but it'll be used in "group per-graph data in ClusterSet", and in "add staging support".
📝 theuni opened a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844)
This makes it possible to build/install only the desired binaries regardless of the configuration.
For consistency, the component names match the binary names. `Kernel` and `GUI` have been renamed.
Additionally it fixes #31762 by installing only the manpages for the configured targets (and includes them in the component installs for each).
Alternative to #31765 which is (imo) more correct/thorough.
Can be tested using (for ex):
```bash
$ cmake -B build
$ cmake --build build -t bitco
...
(https://github.com/bitcoin/bitcoin/pull/31844)
This makes it possible to build/install only the desired binaries regardless of the configuration.
For consistency, the component names match the binary names. `Kernel` and `GUI` have been renamed.
Additionally it fixes #31762 by installing only the manpages for the configured targets (and includes them in the component installs for each).
Alternative to #31765 which is (imo) more correct/thorough.
Can be tested using (for ex):
```bash
$ cmake -B build
$ cmake --build build -t bitco
...
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951571563)
You're right, but that's exactly what the "delay chunking while sub-acceptable" commit is about. In the initial commit, the invariant is that the chunk feerates are maintained at all times (and the sanity check will fail if not). In this "delay chunking while sub-acceptable" commit, some updates are avoided while they are unnecessary. The commit after it ("special-case removal of tail of cluster") reintroduces some, because with that, performing a Split() can in fact immediately introduce optima
...
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951571563)
You're right, but that's exactly what the "delay chunking while sub-acceptable" commit is about. In the initial commit, the invariant is that the chunk feerates are maintained at all times (and the sanity check will fail if not). In this "delay chunking while sub-acceptable" commit, some updates are avoided while they are unnecessary. The commit after it ("special-case removal of tail of cluster") reintroduces some, because with that, performing a Split() can in fact immediately introduce optima
...
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951573976)
No, the order of removals is irrelevant. The comment is just about pointing out that while TxGraph is allowed to reorder dependency additions and transaction removals w.r.t. each other, this doesn't matter in practice. As long as every transaction removal is combined (doesn't matter when) with also removal of all its ancestors, or with all its descendants, this reordering does not affect its observable behavior.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951573976)
No, the order of removals is irrelevant. The comment is just about pointing out that while TxGraph is allowed to reorder dependency additions and transaction removals w.r.t. each other, this doesn't matter in practice. As long as every transaction removal is combined (doesn't matter when) with also removal of all its ancestors, or with all its descendants, this reordering does not affect its observable behavior.
💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951574878)
Same comment as before: in the initial commit, the invariant is that the chunk feerates are maintained at all times. It is relaxed later on.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1951574878)
Same comment as before: in the initial commit, the invariant is that the chunk feerates are maintained at all times. It is relaxed later on.
👍 theuni approved a pull request: "depends: Make default `host` and `build` comparable"
(https://github.com/bitcoin/bitcoin/pull/30584#pullrequestreview-2610020007)
utACK b28917be363fb5a82effffeadbe4ba27bb1c70ce.
Makes sense, thanks for fixing.
(https://github.com/bitcoin/bitcoin/pull/30584#pullrequestreview-2610020007)
utACK b28917be363fb5a82effffeadbe4ba27bb1c70ce.
Makes sense, thanks for fixing.
💬 maflcko commented on pull request "contrib: Add deterministic-fuzz-coverage":
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2652056781)
Fixed the typo. I'll add the other features and a fix for the `m_next_inv_to_inbounds` non-determinism that you mentioned in a follow-up, if that is all right?
(https://github.com/bitcoin/bitcoin/pull/31836#issuecomment-2652056781)
Fixed the typo. I'll add the other features and a fix for the `m_next_inv_to_inbounds` non-determinism that you mentioned in a follow-up, if that is all right?
💬 theuni commented on pull request "depends: Make default `host` and `build` comparable":
(https://github.com/bitcoin/bitcoin/pull/30584#issuecomment-2652074197)
@laanwj Interesting! Looks like cross builds of libevent for kqueue platforms suffer because of the wonky `check_c_source_runs` that can only succeed for native builds. That's probably worth addressing itself.
(https://github.com/bitcoin/bitcoin/pull/30584#issuecomment-2652074197)
@laanwj Interesting! Looks like cross builds of libevent for kqueue platforms suffer because of the wonky `check_c_source_runs` that can only succeed for native builds. That's probably worth addressing itself.
👍 pinheadmz approved a pull request: "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries"
(https://github.com/bitcoin/bitcoin/pull/31407#pullrequestreview-2610053597)
ACK 46e44a35b85830a60cf622e039db19ccf1989008
Tested on arm64/macos, but did not review code or test other platforms. This updated process closes the referenced issues and successfully creates signed binaries for `bitcoind`, `bitcoin-cli` and all other utilities including Bitcoin-Qt.
----
## Detached sigs (`tar xf` and commit locally in `bitcoin-detached-sigs` to test:
[signature-osx-arm64.tar.gz](https://github.com/user-attachments/files/18758816/signature-osx-arm64.tar.gz)
--
...
(https://github.com/bitcoin/bitcoin/pull/31407#pullrequestreview-2610053597)
ACK 46e44a35b85830a60cf622e039db19ccf1989008
Tested on arm64/macos, but did not review code or test other platforms. This updated process closes the referenced issues and successfully creates signed binaries for `bitcoind`, `bitcoin-cli` and all other utilities including Bitcoin-Qt.
----
## Detached sigs (`tar xf` and commit locally in `bitcoin-detached-sigs` to test:
[signature-osx-arm64.tar.gz](https://github.com/user-attachments/files/18758816/signature-osx-arm64.tar.gz)
--
...
💬 pinheadmz commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2652085416)
Bonus verbose code sign verification of `bitcoind`:
```
codesign -dv --verbose=4 /Users/matthewzipkin/Desktop/work/bitcoin/guix-build-46e44a35b858/output/arm64-apple-darwin/arm64-apple-darwin-codesigned/bitcoin-46e44a35b858/bin/bitcoind
Executable=/Users/matthewzipkin/Desktop/work/bitcoin/guix-build-46e44a35b858/output/arm64-apple-darwin/arm64-apple-darwin-codesigned/bitcoin-46e44a35b858/bin/bitcoind
Identifier=bitcoind
Format=Mach-O thin (arm64)
CodeDirectory v=20500 size=23284 flags=0
...
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2652085416)
Bonus verbose code sign verification of `bitcoind`:
```
codesign -dv --verbose=4 /Users/matthewzipkin/Desktop/work/bitcoin/guix-build-46e44a35b858/output/arm64-apple-darwin/arm64-apple-darwin-codesigned/bitcoin-46e44a35b858/bin/bitcoind
Executable=/Users/matthewzipkin/Desktop/work/bitcoin/guix-build-46e44a35b858/output/arm64-apple-darwin/arm64-apple-darwin-codesigned/bitcoin-46e44a35b858/bin/bitcoind
Identifier=bitcoind
Format=Mach-O thin (arm64)
CodeDirectory v=20500 size=23284 flags=0
...
💬 pinheadmz commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1951625820)
not an issue anymore on 46e44a35b85830a60cf622e039db19ccf1989008
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1951625820)
not an issue anymore on 46e44a35b85830a60cf622e039db19ccf1989008
✅ hebasto closed an issue: "Wallet symlinks are not rejected as expected"
(https://github.com/bitcoin/bitcoin/issues/31842)
(https://github.com/bitcoin/bitcoin/issues/31842)
💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2652236274)
> My understanding is that m_total_orphan_usage exists to limit memory usage, and m_total_announcements to limit CPU usage - but why the number of announcements instead of number of orphans?
Yep, to limit CPU usage. The complexity of eviction for example is bounded by the total number of announcements: in the worst case, each orphan has many announcers and the `MaybeTrimOrphans` loop first removes announcements until each orphan just has 1 left, and then finally can remove transactions. See c
...
(https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-2652236274)
> My understanding is that m_total_orphan_usage exists to limit memory usage, and m_total_announcements to limit CPU usage - but why the number of announcements instead of number of orphans?
Yep, to limit CPU usage. The complexity of eviction for example is bounded by the total number of announcements: in the worst case, each orphan has many announcers and the `MaybeTrimOrphans` loop first removes announcements until each orphan just has 1 left, and then finally can remove transactions. See c
...
🤔 mzumsande reviewed a pull request: "validation: set BLOCK_FAILED_CHILD correctly"
(https://github.com/bitcoin/bitcoin/pull/31835#pullrequestreview-2610319771)
Concept ACK
This is a straightforward bugfix.
In general, I'm in favor of removing the `BLOCK_FAILED_CHILD` / `BLOCK_FAILED_VALID` distinction as a follow-up, as suggested in https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-565506585. It's been introduced in 2012 (#1677) and (as far as I know) has never been used for anything. I also can't think of a legitimate use - block indexes that are marked as invalid are invalid indefinitely (outside of the `reconsiderblock` rpc, but that
...
(https://github.com/bitcoin/bitcoin/pull/31835#pullrequestreview-2610319771)
Concept ACK
This is a straightforward bugfix.
In general, I'm in favor of removing the `BLOCK_FAILED_CHILD` / `BLOCK_FAILED_VALID` distinction as a follow-up, as suggested in https://github.com/bitcoin/bitcoin/pull/16856#issuecomment-565506585. It's been introduced in 2012 (#1677) and (as far as I know) has never been used for anything. I also can't think of a legitimate use - block indexes that are marked as invalid are invalid indefinitely (outside of the `reconsiderblock` rpc, but that
...
💬 yancyribbens commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2652350051)
I tracked down the PR that add this: https://github.com/bitcoin/bitcoin/pull/27585 and commit https://github.com/bitcoin/bitcoin/pull/27585/commits/6d9b26d56ab5295dfcfe0f80a3069046a263fb2f. I'm a little confused by this since this is counter to what I would expect. BnB seeks to find a changelss solution, however only if the change created is less than the cost of creating a new output. That is, if it's more expansive to add the change leftover to a new output, don't bother.
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2652350051)
I tracked down the PR that add this: https://github.com/bitcoin/bitcoin/pull/27585 and commit https://github.com/bitcoin/bitcoin/pull/27585/commits/6d9b26d56ab5295dfcfe0f80a3069046a263fb2f. I'm a little confused by this since this is counter to what I would expect. BnB seeks to find a changelss solution, however only if the change created is less than the cost of creating a new output. That is, if it's more expansive to add the change leftover to a new output, don't bother.
👋 achow101's pull request is ready for review: "psbt: MuSig2 Fields"
(https://github.com/bitcoin/bitcoin/pull/31247)
(https://github.com/bitcoin/bitcoin/pull/31247)
💬 achow101 commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2652378647)
Updated with the test vectors from https://github.com/bitcoin/bips/pull/1764
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2652378647)
Updated with the test vectors from https://github.com/bitcoin/bips/pull/1764
💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1951831500)
If I need to retouch
(https://github.com/bitcoin/bitcoin/pull/31407#discussion_r1951831500)
If I need to retouch