💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1900189106)
Updated https://github.com/bitcoin/bitcoin/commit/6a9ec4771d691bf4143f3574dd129da641c6ee74 -> https://github.com/bitcoin/bitcoin/pull/28122/commits/6e7c29528eb4a7c39bf8290b7c622e30bf07b45d ([implement-bip352-00](https://github.com/josibake/bitcoin/tree/implement-bip352-00) -> [implement-bip352-01](https://github.com/josibake/bitcoin/tree/implement-bip352-01), [compare](https://github.com/josibake/bitcoin/compare/implement-bip352-00..implement-bip352-01))
* Adds a test case for taproot inputs wi
...
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1900189106)
Updated https://github.com/bitcoin/bitcoin/commit/6a9ec4771d691bf4143f3574dd129da641c6ee74 -> https://github.com/bitcoin/bitcoin/pull/28122/commits/6e7c29528eb4a7c39bf8290b7c622e30bf07b45d ([implement-bip352-00](https://github.com/josibake/bitcoin/tree/implement-bip352-00) -> [implement-bip352-01](https://github.com/josibake/bitcoin/tree/implement-bip352-01), [compare](https://github.com/josibake/bitcoin/compare/implement-bip352-00..implement-bip352-01))
* Adds a test case for taproot inputs wi
...
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1900227084)
Thank you for the review and detailed comments @ryanofsky!
Rebased e3592ee55756c08bda3075b54cbff719d3fb964f -> 2ef630f3e49f11cbae7abe2dc31c2a908bb5e6ba ([noGlobalSignals_4](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_4) -> [noGlobalSignals_5](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_4..noGlobalSignals_5))
Updated 2ef630f3e49f11cbae7abe2dc31c2a908bb5e6ba -> aad39765ad69565dcbdb
...
(https://github.com/bitcoin/bitcoin/pull/28960#issuecomment-1900227084)
Thank you for the review and detailed comments @ryanofsky!
Rebased e3592ee55756c08bda3075b54cbff719d3fb964f -> 2ef630f3e49f11cbae7abe2dc31c2a908bb5e6ba ([noGlobalSignals_4](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_4) -> [noGlobalSignals_5](https://github.com/TheCharlatan/bitcoin/tree/noGlobalSignals_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalSignals_4..noGlobalSignals_5))
Updated 2ef630f3e49f11cbae7abe2dc31c2a908bb5e6ba -> aad39765ad69565dcbdb
...
💬 willcl-ark commented on issue "macOS App Notarization & Stapling":
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1900233704)
> Given that MacOS is just phoning home all the time, I think we should move ahead with notarizing our MacOS releases to improve the installation experience. Although stapling sounds like a good idea, at this point, I sincerely doubt that Apple will change their tooling to stop phoning home for stapled apps, but maybe we should do so in the hopes that they change their minds.
In this case notarizing MacOS releases seems the most reasonable course of action to me too. Of course we'd like the a
...
(https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1900233704)
> Given that MacOS is just phoning home all the time, I think we should move ahead with notarizing our MacOS releases to improve the installation experience. Although stapling sounds like a good idea, at this point, I sincerely doubt that Apple will change their tooling to stop phoning home for stapled apps, but maybe we should do so in the hopes that they change their minds.
In this case notarizing MacOS releases seems the most reasonable course of action to me too. Of course we'd like the a
...
💬 TheCharlatan commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1458838935)
> It might also be worth asking whether the signals parameter actually should be required.
It is used in `CTxMemPool` methods, so should be required, no? Or are you saying these classes should be given the option to not be sending signals in the first place? In that case, I don't see what the difference between the mempool and the other validation classes is. The signals could be optional in the chainman too.
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1458838935)
> It might also be worth asking whether the signals parameter actually should be required.
It is used in `CTxMemPool` methods, so should be required, no? Or are you saying these classes should be given the option to not be sending signals in the first place? In that case, I don't see what the difference between the mempool and the other validation classes is. The signals could be optional in the chainman too.
📝 hebasto opened a pull request: "depends: Ensure definitions are passed when building SQLite with `DEBUG=1`"
(https://github.com/bitcoin/bitcoin/pull/29282)
The SQLite build system overrides the `CFLAGS` when is configured with the `--enable-debug` option.
Amends https://github.com/bitcoin/bitcoin/pull/25987.
Otherwise, for example in OSS-Fuzz, the testing binaries diverge from release ones.
(https://github.com/bitcoin/bitcoin/pull/29282)
The SQLite build system overrides the `CFLAGS` when is configured with the `--enable-debug` option.
Amends https://github.com/bitcoin/bitcoin/pull/25987.
Otherwise, for example in OSS-Fuzz, the testing binaries diverge from release ones.
💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1458922669)
re: https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1458838935
> The signals could be optional in the chainman too.
Yes, absolutely. The question is in cases where libbitcoinkernel applications don't care about receiving signals (if maybe just they are just validating a block), should they be forced to declare `ValidationSignals validation_signals{std::make_unique<util::ImmediateTaskRunner>()};` and pass a `validation_signals` parameter to the ChainstateManager object that won't
...
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1458922669)
re: https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1458838935
> The signals could be optional in the chainman too.
Yes, absolutely. The question is in cases where libbitcoinkernel applications don't care about receiving signals (if maybe just they are just validating a block), should they be forced to declare `ValidationSignals validation_signals{std::make_unique<util::ImmediateTaskRunner>()};` and pass a `validation_signals` parameter to the ChainstateManager object that won't
...
💬 sipa commented on issue "Post startup stalling":
(https://github.com/bitcoin/bitcoin/issues/29281#issuecomment-1900333798)
Discussed this briefly with @mzumsande.
The stalling detection logic is ineffective here, because that only triggers when the 1024-block wide download window cannot move, and as you're less than 1024 blocks behind, the window is already as far as it can go.
We should probably have a separate criterion for stalling detection in this case.
(https://github.com/bitcoin/bitcoin/issues/29281#issuecomment-1900333798)
Discussed this briefly with @mzumsande.
The stalling detection logic is ineffective here, because that only triggers when the 1024-block wide download window cannot move, and as you're less than 1024 blocks behind, the window is already as far as it can go.
We should probably have a separate criterion for stalling detection in this case.
💬 TheCharlatan commented on pull request "depends: add NM output to gen_id":
(https://github.com/bitcoin/bitcoin/pull/29249#issuecomment-1900352612)
Yeah, also built on the wrong commit, so:
Re-ACK 6ec2813cd88d5f0b955d746e4711a8ad256ea47f
Guix build (x86):
```
67d1d5dd27bc7b2df6b5d77265a9be0c5e3d7cf0bd003b8ec177010c18526814 guix-build-6ec2813cd88d/output/aarch64-linux-gnu/SHA256SUMS.part
59d2f605151251ee05e9c8b043ead799254c0b61da513df8ba4b9adb8c3dd619 guix-build-6ec2813cd88d/output/aarch64-linux-gnu/bitcoin-6ec2813cd88d-aarch64-linux-gnu-debug.tar.gz
d299e058786d3db9e145ea692b4e90b9ce8a60ec16d101e475e8b2927b53f5ac guix-build-6ec28
...
(https://github.com/bitcoin/bitcoin/pull/29249#issuecomment-1900352612)
Yeah, also built on the wrong commit, so:
Re-ACK 6ec2813cd88d5f0b955d746e4711a8ad256ea47f
Guix build (x86):
```
67d1d5dd27bc7b2df6b5d77265a9be0c5e3d7cf0bd003b8ec177010c18526814 guix-build-6ec2813cd88d/output/aarch64-linux-gnu/SHA256SUMS.part
59d2f605151251ee05e9c8b043ead799254c0b61da513df8ba4b9adb8c3dd619 guix-build-6ec2813cd88d/output/aarch64-linux-gnu/bitcoin-6ec2813cd88d-aarch64-linux-gnu-debug.tar.gz
d299e058786d3db9e145ea692b4e90b9ce8a60ec16d101e475e8b2927b53f5ac guix-build-6ec28
...
💬 1440000bytes commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1900364519)
> This isn't Twitter, if you want to participate here make an effort to read everything through and through.
Exactly, this isn't twitter so a detailed explanation with the ACK helps everyone and adds more weight to your comment.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1900364519)
> This isn't Twitter, if you want to participate here make an effort to read everything through and through.
Exactly, this isn't twitter so a detailed explanation with the ACK helps everyone and adds more weight to your comment.
👍 TheCharlatan approved a pull request: "refactor: remove CTxMemPool::queryHashes()"
(https://github.com/bitcoin/bitcoin/pull/29260#pullrequestreview-1832517919)
ACK a30af58d26ec98350ae273ad10793a12ad71cff6
(https://github.com/bitcoin/bitcoin/pull/29260#pullrequestreview-1832517919)
ACK a30af58d26ec98350ae273ad10793a12ad71cff6
💬 moonsettler commented on pull request "Implement OP_CHECKTEMPLATEVERIFY":
(https://github.com/bitcoin/bitcoin/pull/29280#issuecomment-1900407815)
Concept ACK!
(https://github.com/bitcoin/bitcoin/pull/29280#issuecomment-1900407815)
Concept ACK!
💬 moonsettler commented on pull request "Implement OP_CHECKSIGFROMSTACK(VERIFY)":
(https://github.com/bitcoin/bitcoin/pull/29270#issuecomment-1900408113)
Concept ACK!
(https://github.com/bitcoin/bitcoin/pull/29270#issuecomment-1900408113)
Concept ACK!
💬 moonsettler commented on pull request "Add OP_INTERNALKEY for Tapscript":
(https://github.com/bitcoin/bitcoin/pull/29269#issuecomment-1900408304)
Concept ACK!
(https://github.com/bitcoin/bitcoin/pull/29269#issuecomment-1900408304)
Concept ACK!
🚀 fanquake merged a pull request: "depends: add NM output to gen_id"
(https://github.com/bitcoin/bitcoin/pull/29249)
(https://github.com/bitcoin/bitcoin/pull/29249)
🤔 furszy reviewed a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1832560555)
See #28170. Obvious concept ACK.
We do have a post-IBD bug on this process.
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1832560555)
See #28170. Obvious concept ACK.
We do have a post-IBD bug on this process.
💬 vasild commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1900458347)
> HasAllDesirableServiceFlags(), relies on IsInitialBlockDownload() / DEFAULT_MAX_TIP_AGE (24h) on master
...
> I think this means that the safety buffer of 144 blocks proposed in [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki) would have been removed.
I do not think `-maxtipage` / `DEFAULT_MAX_TIP_AGE` implements "the safety buffer" from BIP159. The former was introduced about 2 years before BIP159, in 2015: https://github.com/bitcoin/bitcoin/pull/5987.
BIP159 r
...
(https://github.com/bitcoin/bitcoin/pull/28170#issuecomment-1900458347)
> HasAllDesirableServiceFlags(), relies on IsInitialBlockDownload() / DEFAULT_MAX_TIP_AGE (24h) on master
...
> I think this means that the safety buffer of 144 blocks proposed in [BIP159](https://github.com/bitcoin/bips/blob/master/bip-0159.mediawiki) would have been removed.
I do not think `-maxtipage` / `DEFAULT_MAX_TIP_AGE` implements "the safety buffer" from BIP159. The former was introduced about 2 years before BIP159, in 2015: https://github.com/bitcoin/bitcoin/pull/5987.
BIP159 r
...
💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1900470464)
Here's a godbolt test that shows how various compilers perform: https://gcc.godbolt.org/z/nTadqEP83
To test with/without builtins, comment/un-comment the `DISABLE_BUILTIN_BSWAPS` define at line 14.
From my tests:
- clang: all c++20 supporting versions (>= 10.0) optimize without the need for builtins
- gcc: no version optimizes without the builtins
- msvc: versions >= 19.37 optimize without builtins
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1900470464)
Here's a godbolt test that shows how various compilers perform: https://gcc.godbolt.org/z/nTadqEP83
To test with/without builtins, comment/un-comment the `DISABLE_BUILTIN_BSWAPS` define at line 14.
From my tests:
- clang: all c++20 supporting versions (>= 10.0) optimize without the need for builtins
- gcc: no version optimizes without the builtins
- msvc: versions >= 19.37 optimize without builtins
💬 jamesob commented on pull request "Update libsecp256k1 subtree to current master":
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1900505846)
Oddly enough, even with `gcc 12.2.0`, the regression on the first machine holds. Artifacts here: https://gist.github.com/jamesob/1f4456f1f9bafcabb392210bbfe9f240
Will run the secp benches in isolation.
(https://github.com/bitcoin/bitcoin/pull/29169#issuecomment-1900505846)
Oddly enough, even with `gcc 12.2.0`, the regression on the first machine holds. Artifacts here: https://gist.github.com/jamesob/1f4456f1f9bafcabb392210bbfe9f240
Will run the secp benches in isolation.
📝 stickies-v opened a pull request: "test: ensure output is large enough to pay for its fees"
(https://github.com/bitcoin/bitcoin/pull/29283)
Fixes a (rare) intermittency issue in wallet_import_rescan.py
Since we [use](https://github.com/bitcoin/bitcoin/blob/03752444cd54df05a731557968d5a9f33a55c55c/test/functional/wallet_import_rescan.py#L296) `subtract_fee_from_outputs=[0]` in the `send` command, the output amount must at least be as large as the fee we're paying.
Example in CI: https://api.cirrus-ci.com/v1/task/6107972259020800/logs/ci.log
```
2024-01-18T22:16:12.383000Z TestFramework (INFO): Test that the mempool is resca
...
(https://github.com/bitcoin/bitcoin/pull/29283)
Fixes a (rare) intermittency issue in wallet_import_rescan.py
Since we [use](https://github.com/bitcoin/bitcoin/blob/03752444cd54df05a731557968d5a9f33a55c55c/test/functional/wallet_import_rescan.py#L296) `subtract_fee_from_outputs=[0]` in the `send` command, the output amount must at least be as large as the fee we're paying.
Example in CI: https://api.cirrus-ci.com/v1/task/6107972259020800/logs/ci.log
```
2024-01-18T22:16:12.383000Z TestFramework (INFO): Test that the mempool is resca
...
💬 stickies-v commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1900516149)
Reviewers might be interested in https://github.com/bitcoin/bitcoin/pull/29283, fixing a rare intermittency issue
(https://github.com/bitcoin/bitcoin/pull/29179#issuecomment-1900516149)
Reviewers might be interested in https://github.com/bitcoin/bitcoin/pull/29283, fixing a rare intermittency issue