🤔 rkrux reviewed a pull request: "validation: Improve warnings in case of chain corruption"
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3524622915)
Concept ACK 53ef86f0ecd4ca2b66c4e3dce591ac4718e38f0e
Adding the warning in such a case should help the user in diagnosing the issue; I have limited exposure to this area, hence refraining from giving a full a-c-k.
(https://github.com/bitcoin/bitcoin/pull/33553#pullrequestreview-3524622915)
Concept ACK 53ef86f0ecd4ca2b66c4e3dce591ac4718e38f0e
Adding the warning in such a case should help the user in diagnosing the issue; I have limited exposure to this area, hence refraining from giving a full a-c-k.
💬 rkrux commented on pull request "validation: Improve warnings in case of chain corruption":
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2576795045)
Looking forward to this functional test!
(https://github.com/bitcoin/bitcoin/pull/33553#discussion_r2576795045)
Looking forward to this functional test!
👍 rkrux approved a pull request: "wallet: warn against accidental unsafe older() import"
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3524889277)
lgtm ACK 189437a3dedefce3945ab2a8c05785eb283c2c6f
Shared a non-blocking nit.
(https://github.com/bitcoin/bitcoin/pull/33135#pullrequestreview-3524889277)
lgtm ACK 189437a3dedefce3945ab2a8c05785eb283c2c6f
Shared a non-blocking nit.
💬 rkrux commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2576993777)
Nit: some consistency in the warning format would be easier on the eyes.
```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 8289120164..132d695a1a 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1557,11 +1557,11 @@ public:
const uint32_t raw = node.k;
const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
-
...
(https://github.com/bitcoin/bitcoin/pull/33135#discussion_r2576993777)
Nit: some consistency in the warning format would be easier on the eyes.
```diff
diff --git a/src/script/descriptor.cpp b/src/script/descriptor.cpp
index 8289120164..132d695a1a 100644
--- a/src/script/descriptor.cpp
+++ b/src/script/descriptor.cpp
@@ -1557,11 +1557,11 @@ public:
const uint32_t raw = node.k;
const uint32_t value_part = raw & ~CTxIn::SEQUENCE_LOCKTIME_TYPE_FLAG;
if (value_part > CTxIn::SEQUENCE_LOCKTIME_MASK) {
-
...
👍 l0rinc approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3524866538)
The latest version splits out a few trivial moves with explanations to unburden heavier commits. I particularly like how `PrivateBroadcast` methods are added commit by commit when they're actually needed.
The signal-to-noise ratio is also improved significantly now - irrelevant debug logs aren't dominating anymore.
We also finally found a way to write the priority comparator in an intuitive way that the CI gods didn't reject!
`GetHash()` calls for hash and equality have been migrated to witne
...
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3524866538)
The latest version splits out a few trivial moves with explanations to unburden heavier commits. I particularly like how `PrivateBroadcast` methods are added commit by commit when they're actually needed.
The signal-to-noise ratio is also improved significantly now - irrelevant debug logs aren't dominating anymore.
We also finally found a way to write the priority comparator in an intuitive way that the CI gods didn't reject!
`GetHash()` calls for hash and equality have been migrated to witne
...
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2577109437)
to get 100% coverage of `PrivateBroadcast`, we could add:
```C++
BOOST_CHECK(!pb.DidNodeConfirmReception(nonexistent_recipient));
```
to exercise:
https://github.com/bitcoin/bitcoin/blob/2d398050ee31db05e9222149b5e60572ac31d803/src/private_broadcast.cpp#L82
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2577109437)
to get 100% coverage of `PrivateBroadcast`, we could add:
```C++
BOOST_CHECK(!pb.DidNodeConfirmReception(nonexistent_recipient));
```
to exercise:
https://github.com/bitcoin/bitcoin/blob/2d398050ee31db05e9222149b5e60572ac31d803/src/private_broadcast.cpp#L82
💬 l0rinc commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2576975959)
nit:
```suggestion
* Avoid calling this for other peers since it will degrade privacy.
```
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2576975959)
nit:
```suggestion
* Avoid calling this for other peers since it will degrade privacy.
```
💬 maflcko commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2577151438)
> Nit: The `callback_set` variable seems unnecessary now.
Why would it be unnecessary now? Sure, it is harmless to call `SyncWithValidationInterfaceQueue` when the tx is in the mempool, but this will need to be mentioned
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2577151438)
> Nit: The `callback_set` variable seems unnecessary now.
Why would it be unnecessary now? Sure, it is harmless to call `SyncWithValidationInterfaceQueue` when the tx is in the mempool, but this will need to be mentioned
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577234887)
doc: Add design notes for cluster mempool and explain new mempool limits
Grammar: "we can can compare" -> "we can compare"
(thanks DrahtBot LLM)
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577234887)
doc: Add design notes for cluster mempool and explain new mempool limits
Grammar: "we can can compare" -> "we can compare"
(thanks DrahtBot LLM)
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577225993)
In commit "doc: Add design notes for cluster mempool and explain new mempool limits"
With these options made debug-only, maybe it's better to list the defaults?
> Transactions submitted to the mempool must not result in clusters that would
exceed the cluster limits (64 transactions and 101 kvB total per cluster).
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577225993)
In commit "doc: Add design notes for cluster mempool and explain new mempool limits"
With these options made debug-only, maybe it's better to list the defaults?
> Transactions submitted to the mempool must not result in clusters that would
exceed the cluster limits (64 transactions and 101 kvB total per cluster).
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577238632)
In commit "doc: add release notes snippet for cluster mempool"
"overriden" -> "overridden"
(thanks to DrahtBot LLM)
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577238632)
In commit "doc: add release notes snippet for cluster mempool"
"overriden" -> "overridden"
(thanks to DrahtBot LLM)
💬 sipa commented on pull request "Cluster mempool followups":
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577251357)
In commit "rpc: improve getmempoolcluster output"
`all_chunks.push_back(std::move(chunk))` will avoid a copy
(https://github.com/bitcoin/bitcoin/pull/33591#discussion_r2577251357)
In commit "rpc: improve getmempoolcluster output"
`all_chunks.push_back(std::move(chunk))` will avoid a copy
💬 billymcbip commented on pull request "script: Remove dead code from OP_CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33977#issuecomment-3596782767)
I think it wouldn't hurt to have an `assert(!stack.empty());` instead, just in case the code above is ever refactored.
(https://github.com/bitcoin/bitcoin/pull/33977#issuecomment-3596782767)
I think it wouldn't hurt to have an `assert(!stack.empty());` instead, just in case the code above is ever refactored.
🤔 sipa reviewed a pull request: "Cluster mempool followups"
(https://github.com/bitcoin/bitcoin/pull/33591#pullrequestreview-3525225192)
ACK 48c29fbe0974664e28564af07fbfd753cf125c36
See nits above.
(https://github.com/bitcoin/bitcoin/pull/33591#pullrequestreview-3525225192)
ACK 48c29fbe0974664e28564af07fbfd753cf125c36
See nits above.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577293052)
That would be racy. We could have the worker thread count be a variable instead of hard coded, then for a test we could make it zero.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577293052)
That would be racy. We could have the worker thread count be a variable instead of hard coded, then for a test we could make it zero.
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577297663)
Oh, we override so we make sure all threads are stopped before we do the batch write. This is in a comment two lines below.
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577297663)
Oh, we override so we make sure all threads are stopped before we do the batch write. This is in a comment two lines below.
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577299367)
Looks like this needs more work, it's not as easy as I though - we could use an `std::deque` instead of a `vector` here:
```patch
Subject: [PATCH] std::deque<InputToFetch> m_inputs
---
diff --git a/src/coinsviewcacheasync.h b/src/coinsviewcacheasync.h
--- a/src/coinsviewcacheasync.h (revision a3f56354d6e3f64eaca84a16e4951e6073090f60)
+++ b/src/coinsviewcacheasync.h (revision 462408b897197de3a7067dcbdee318ad9dc1e546)
@@ -17,6 +17,7 @@
#include <atomic>
#include <barrier>
#include <cs
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577299367)
Looks like this needs more work, it's not as easy as I though - we could use an `std::deque` instead of a `vector` here:
```patch
Subject: [PATCH] std::deque<InputToFetch> m_inputs
---
diff --git a/src/coinsviewcacheasync.h b/src/coinsviewcacheasync.h
--- a/src/coinsviewcacheasync.h (revision a3f56354d6e3f64eaca84a16e4951e6073090f60)
+++ b/src/coinsviewcacheasync.h (revision 462408b897197de3a7067dcbdee318ad9dc1e546)
@@ -17,6 +17,7 @@
#include <atomic>
#include <barrier>
#include <cs
...
💬 vostrnad commented on pull request "script: Remove dead code from OP_CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33977#issuecomment-3596822343)
The script interpreter code is so thoroughly tested, so infrequently refactored, and any change to it reviewed with such extreme caution that I don't think we need to bother with an assert, but I can add it if others think it's a good idea.
(https://github.com/bitcoin/bitcoin/pull/33977#issuecomment-3596822343)
The script interpreter code is so thoroughly tested, so infrequently refactored, and any change to it reviewed with such extreme caution that I don't think we need to bother with an assert, but I can add it if others think it's a good idea.
💬 l0rinc commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577321551)
you sure it would be racy?
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577321551)
you sure it would be racy?
💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577345226)
> I disagree, I think the current move construction is incorrect and if I understand it correctly, we should reserve instead and delete (or ignore) the other constructors.
I think you are disagreeing with my statement:
> since it only happens before we start doing work, might as well make it simple and not bother moving the other fields
because the other things I said were facts. Do you think we should also construct new `atomic_flag`s when doing the move construction? As I mentioned I
...
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577345226)
> I disagree, I think the current move construction is incorrect and if I understand it correctly, we should reserve instead and delete (or ignore) the other constructors.
I think you are disagreeing with my statement:
> since it only happens before we start doing work, might as well make it simple and not bother moving the other fields
because the other things I said were facts. Do you think we should also construct new `atomic_flag`s when doing the move construction? As I mentioned I
...