🤔 Prabhat1308 reviewed a pull request: "test: clarify timewarp grace period griefing attack"
(https://github.com/bitcoin/bitcoin/pull/31725#pullrequestreview-2609845672)
ACK [`6065b78`](https://github.com/bitcoin/bitcoin/pull/31725/commits/6065b78e22e47f053227f68afb8e3afa7bf0ab95)
(https://github.com/bitcoin/bitcoin/pull/31725#pullrequestreview-2609845672)
ACK [`6065b78`](https://github.com/bitcoin/bitcoin/pull/31725/commits/6065b78e22e47f053227f68afb8e3afa7bf0ab95)
💬 hodlinator commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2651918799)
Just reproduced the same failure using binaries produced by CI cross-compilation setup from #31176 as I had before using the [Guix](https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753) cross-compiled build. Good to see that the two have the same behavior... but worrying that nobody else is getting this.
```
$ BITCOINCLI=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoin-cli.exe BITCOIND=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoind.exe build/test/functional/wallet_
...
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2651918799)
Just reproduced the same failure using binaries produced by CI cross-compilation setup from #31176 as I had before using the [Guix](https://github.com/bitcoin/bitcoin/pull/31410#pullrequestreview-2604068753) cross-compiled build. Good to see that the two have the same behavior... but worrying that nobody else is getting this.
```
$ BITCOINCLI=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoin-cli.exe BITCOIND=/c/Users/hodlinator/Downloads/18274b6-bins/bitcoind.exe build/test/functional/wallet_
...
🤔 Prabhat1308 reviewed a pull request: "test: check `scanning` field from `getwalletinfo`"
(https://github.com/bitcoin/bitcoin/pull/31768#pullrequestreview-2609887988)
re-ACK [`bb0879d`](https://github.com/bitcoin/bitcoin/pull/31768/commits/bb0879ddabc8b3a7253bc774d23b842937d18015)
(https://github.com/bitcoin/bitcoin/pull/31768#pullrequestreview-2609887988)
re-ACK [`bb0879d`](https://github.com/bitcoin/bitcoin/pull/31768/commits/bb0879ddabc8b3a7253bc774d23b842937d18015)
💬 theuni commented on pull request "cmake: Install man pages for configured targets only":
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2651979630)
> Considering that in order to hook up `COMPONENTS` usage, we'd need to add an `install()` line for each as opposed to our current `installable_targets` approach, I'm not sure that `install_manpage` is the right abstraction.
>
> Instead, I think we're going to want an `add_install()` or so, right?
See [this branch](https://github.com/theuni/bitcoin/commits/cmake-install-components/) for an alternative which adds `install_binary_component` as described above. Imo it's more thorough/correct.
...
(https://github.com/bitcoin/bitcoin/pull/31765#issuecomment-2651979630)
> Considering that in order to hook up `COMPONENTS` usage, we'd need to add an `install()` line for each as opposed to our current `installable_targets` approach, I'm not sure that `install_manpage` is the right abstraction.
>
> Instead, I think we're going to want an `add_install()` or so, right?
See [this branch](https://github.com/theuni/bitcoin/commits/cmake-install-components/) for an alternative which adds `install_binary_component` as described above. Imo it's more thorough/correct.
...
💬 theStack commented on pull request "contrib: fix `test_deterministic_coverage.sh` script for out-of-tree builds":
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2651994364)
> > I wonder if it wouldn't be easier to just go with clang/llvm
>
> I've tested the suggested diff and it worked for me. llvm/clang is my default anyway so I would give this a Concept ACK if you want to follow this approach @theStack
Concept ACK on switching over to clang/llvm as well considering all the reported problems. Note though that this PR merely tries to restore the previous behaviour of the script for CMake builds (and minor improvements that are enabled by that), so I think a d
...
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2651994364)
> > I wonder if it wouldn't be easier to just go with clang/llvm
>
> I've tested the suggested diff and it worked for me. llvm/clang is my default anyway so I would give this a Concept ACK if you want to follow this approach @theStack
Concept ACK on switching over to clang/llvm as well considering all the reported problems. Note though that this PR merely tries to restore the previous behaviour of the script for CMake builds (and minor improvements that are enabled by that), so I think a d
...
💬 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-2652004183)
Sure, but it may take some time until I get around to it.
(https://github.com/bitcoin/bitcoin/pull/31588#issuecomment-2652004183)
Sure, but it may take some time until I get around to it.
💬 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)