💬 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) {
-
...

👍 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
...

💬 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

💬 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.
```

💬 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

💬 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)

💬 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).

💬 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)

💬 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

💬 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.

🤔 sipa reviewed a pull request: "Cluster mempool followups"
(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.

💬 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.

💬 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
...

💬 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.

💬 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?

💬 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
...

💬 fanquake commented on pull request "guix: reduce allowed exported symbols":
(https://github.com/bitcoin/bitcoin/pull/33950#issuecomment-3596900808)
Guix Build (aarch64):
```bash
b507c965417a1adcd6b10000f9a8bae29e18ba9a1e76060c38af7fe1fbbcd0e4 guix-build-7b90b4f5bb10/output/aarch64-linux-gnu/SHA256SUMS.part
c60c33e4ececbc650bbeda06c7aa7dad714ace312a08c804afd750fc0a0c3d68 guix-build-7b90b4f5bb10/output/aarch64-linux-gnu/bitcoin-7b90b4f5bb10-aarch64-linux-gnu-debug.tar.gz
77bdfc199c87064ee166c6c124aa9d586fb24b144d8408bdd3d0e2e0065577a5 guix-build-7b90b4f5bb10/output/aarch64-linux-gnu/bitcoin-7b90b4f5bb10-aarch64-linux-gnu.tar.gz
5b6c18
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577362125)
There is a `ConnectBlock` benchmark already. But, it only does internal spends so it won't be that great for this. I can maybe extend it to also work on a block with inputs from leveldb. WDYT?

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577369275)
If we have a backing cache that blocks, how can we know if it's the main thread or worker threads that need to be blocked? And if we block the main thread by mistake, it will make no progress even though the worker thread can fetch all inputs

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads >40% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r2577376037)
We want to specifically test that we do not access the main cache's backing view, by e.g. calling `GetCoin` on it. If we return a nullopt here then it would correctly go to the db layer, while we want to make sure we blow up here because it is a failed test.