💬 vasild commented on pull request "wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it":
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1899991600)
> Would suggest replacing PR title and description with...
Done. Thanks for the suggestion!
(https://github.com/bitcoin/bitcoin/pull/28774#issuecomment-1899991600)
> Would suggest replacing PR title and description with...
Done. Thanks for the suggestion!
💬 Olayiwolaxiii commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1899994384)
> When disabling the "test-only" assumptions in CheckBlockIndex, the check fails. This is problematic, because test-only code should not affect the behavior of the program in production.
>
>
>
> Current diff:
>
>
>
> ```diff
>
> diff --git a/src/validation.cpp b/src/validation.cpp
>
> index 8c583c586c..00d7ee3736 100644
>
> --- a/src/validation.cpp
>
> +++ b/src/validation.cpp
>
> @@ -4866,9 +4866,9 @@ void ChainstateManager::CheckBlockIndex()
>
> unsigned int prev_chain_tx
...
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1899994384)
> When disabling the "test-only" assumptions in CheckBlockIndex, the check fails. This is problematic, because test-only code should not affect the behavior of the program in production.
>
>
>
> Current diff:
>
>
>
> ```diff
>
> diff --git a/src/validation.cpp b/src/validation.cpp
>
> index 8c583c586c..00d7ee3736 100644
>
> --- a/src/validation.cpp
>
> +++ b/src/validation.cpp
>
> @@ -4866,9 +4866,9 @@ void ChainstateManager::CheckBlockIndex()
>
> unsigned int prev_chain_tx
...
👍 delta1 approved a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1831909216)
Code Review ACK 358561e54ef84f64b216ca8a4271ad17d91b1d51
Adds a new test, increases code coverage.
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1831909216)
Code Review ACK 358561e54ef84f64b216ca8a4271ad17d91b1d51
Adds a new test, increases code coverage.
💬 vasild commented on pull request "wallet: avoid returning a reference to vMasterKey after releasing the mutex that guards it":
(https://github.com/bitcoin/bitcoin/pull/28774#discussion_r1458625183)
Will do if I retouch.
(https://github.com/bitcoin/bitcoin/pull/28774#discussion_r1458625183)
Will do if I retouch.
💬 t-bast commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1900036446)
> No you can’t spend in-mempool htlc outputs 1 CSV post-anchor. Assuming no 1 CSV, yes in my scenario.
We're analyzing this with the changes related to v3, in which we will remove the `CSV 1`.
> However attacker can construct revoked or valid pinning state to have the 483 outputs as inbound HTLC to inflate. Alice no preimage to spend them.
But Alice doesn't need the preimage to spend them, this is a revoked commitment, she spends through the revocation path. Unless you're thinking of a
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1900036446)
> No you can’t spend in-mempool htlc outputs 1 CSV post-anchor. Assuming no 1 CSV, yes in my scenario.
We're analyzing this with the changes related to v3, in which we will remove the `CSV 1`.
> However attacker can construct revoked or valid pinning state to have the 483 outputs as inbound HTLC to inflate. Alice no preimage to spend them.
But Alice doesn't need the preimage to spend them, this is a revoked commitment, she spends through the revocation path. Unless you're thinking of a
...
💬 Xaspr commented on issue "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error":
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1900056556)
Core just fully synced with the changes mentioned earlier:
> Reinstalled v26.0.0.
> Deleted antivirus software Bitdefender.
> Enabled Microsoft Defender, with an exclusion for the %appdata%/Bitcoin folder.
> Excluded bitcoin-qt for exploit mitigation in HitmanPro.Alert.
> Added the following lines in bitcoin.conf:
> connect=<<other node on LAN>>
> assumevalid=0
> dbcache=24000
> disablewallet=1
>
The issue wasn't caused by hardware, but must have been caused by antivirus softwa
...
(https://github.com/bitcoin/bitcoin/issues/29255#issuecomment-1900056556)
Core just fully synced with the changes mentioned earlier:
> Reinstalled v26.0.0.
> Deleted antivirus software Bitdefender.
> Enabled Microsoft Defender, with an exclusion for the %appdata%/Bitcoin folder.
> Excluded bitcoin-qt for exploit mitigation in HitmanPro.Alert.
> Added the following lines in bitcoin.conf:
> connect=<<other node on LAN>>
> assumevalid=0
> dbcache=24000
> disablewallet=1
>
The issue wasn't caused by hardware, but must have been caused by antivirus softwa
...
✅ Xaspr closed an issue: "Unable to sync blockchain on laptop: ERROR: ReadBlockFromDisk: Deserialize or I/O error"
(https://github.com/bitcoin/bitcoin/issues/29255)
(https://github.com/bitcoin/bitcoin/issues/29255)
⚠️ fanquake opened an issue: "Post startup stalling"
(https://github.com/bitcoin/bitcoin/issues/29281)
Noticed this on a node recently. Basically startup, sync two blocks, but then stall for 15 minutes, until we timed out a peer (in this case a blocks-only peer). @mzumsande mentioned this is because we were too close to tip, for the stalling logic to kick in, but also not close enough for the parallel download logic to kick in. I'm wondering if we should adjust the thresholds for either, to try and make this less likely to occur. It's possible this is also the same issue being seen here: https://
...
(https://github.com/bitcoin/bitcoin/issues/29281)
Noticed this on a node recently. Basically startup, sync two blocks, but then stall for 15 minutes, until we timed out a peer (in this case a blocks-only peer). @mzumsande mentioned this is because we were too close to tip, for the stalling logic to kick in, but also not close enough for the parallel download logic to kick in. I'm wondering if we should adjust the thresholds for either, to try and make this less likely to occur. It's possible this is also the same issue being seen here: https://
...
💬 hebasto commented on pull request "build: Pass sanitize flags to instrument `libsecp256k1` code":
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1458717675)
Sure. Done.
(https://github.com/bitcoin/bitcoin/pull/28875#discussion_r1458717675)
Sure. Done.
💬 fanquake commented on pull request "[26.x] more backports":
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1900132316)
Lets add #29127, so if we decide to, we can test macOS notarization: https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1899228781.
(https://github.com/bitcoin/bitcoin/pull/29209#issuecomment-1900132316)
Lets add #29127, so if we decide to, we can test macOS notarization: https://github.com/bitcoin/bitcoin/issues/15774#issuecomment-1899228781.
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1900134591)
Rebased https://github.com/bitcoin/bitcoin/commit/0299dfb0607b85813c7226690772ab3be198b0df -> https://github.com/bitcoin/bitcoin/commit/6a9ec4771d691bf4143f3574dd129da641c6ee74
* Fixes a silent merge conflict introduced by changing `CKey` to use `std::byte*` instead of `unsigned char`
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1900134591)
Rebased https://github.com/bitcoin/bitcoin/commit/0299dfb0607b85813c7226690772ab3be198b0df -> https://github.com/bitcoin/bitcoin/commit/6a9ec4771d691bf4143f3574dd129da641c6ee74
* Fixes a silent merge conflict introduced by changing `CKey` to use `std::byte*` instead of `unsigned char`
👍 fanquake approved a pull request: "depends: Update libmultiprocess library to fix C++20 macos build error"
(https://github.com/bitcoin/bitcoin/pull/29276#pullrequestreview-1832088849)
ACK 5dfd24581a7c5497601966a5371d8c33eabcee8a - thanks. Checked this fixes the macOS build. Also tested the build on aarch64.
(https://github.com/bitcoin/bitcoin/pull/29276#pullrequestreview-1832088849)
ACK 5dfd24581a7c5497601966a5371d8c33eabcee8a - thanks. Checked this fixes the macOS build. Also tested the build on aarch64.
💬 RicYashiroLee commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1900148350)
> Concept NACK.
>
> Bitcoin moves value* through time + space.
>
> *Value is subjective. (Value, in Bitcoin, is subjectively determined by the fee-rate of the tx.)
>
> Bitcoin is a cypherpunk system. Bitcoin is based on crypto-anarchy. This is a colorful way of saying that Bitcoin is about freedom, including free markets.
>
> Bitcoin's anti-DoS mechanism is not about Bitcoin Core sending political messages to dictate to the economy what it should or should not do.
>
> Bitcoin's an
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1900148350)
> Concept NACK.
>
> Bitcoin moves value* through time + space.
>
> *Value is subjective. (Value, in Bitcoin, is subjectively determined by the fee-rate of the tx.)
>
> Bitcoin is a cypherpunk system. Bitcoin is based on crypto-anarchy. This is a colorful way of saying that Bitcoin is about freedom, including free markets.
>
> Bitcoin's anti-DoS mechanism is not about Bitcoin Core sending political messages to dictate to the economy what it should or should not do.
>
> Bitcoin's an
...
💬 fanquake commented on pull request "depends: add NM output to gen_id":
(https://github.com/bitcoin/bitcoin/pull/29249#issuecomment-1900188495)
@TheCharlatan note that you've ACK'd what I assume was the prior commit hash here
(https://github.com/bitcoin/bitcoin/pull/29249#issuecomment-1900188495)
@TheCharlatan note that you've ACK'd what I assume was the prior commit hash here
💬 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
...