💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822219540)
> ACK [dda90a9](https://github.com/bitcoin/bitcoin/commit/dda90a96bd01e34f4a564fbd15f1948f1c22ff58)
>
> Nice find!
>
> > external non-ranged descriptor MUST be able to have label (current impl disallows it)
>
> Since we already have a test for this and this PR doesn't change it, could you clarify what you mean?

the test in question:
```
self.log.info("Should import a p2pkh descriptor")
import_request = {"desc": descsum_create("pkh(" + key.pubkey + ")"),

...

💬 scgbckbone commented on pull request "wallet: allow label for non-ranged external descriptor & disallow label for ranged descriptors":
(https://github.com/bitcoin/bitcoin/pull/31514#issuecomment-2822221513)
re-based on current master

💬 l0rinc commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822227361)
ACK faca46b0421b568e7e5fefe593420e773d0ec9af

🤔 rkrux reviewed a pull request: "test: fix pushdata scripts"
(https://github.com/bitcoin/bitcoin/pull/32270#pullrequestreview-2785087703)
Concept ACK

My bad I didn't look at the scripts thoroughly in the previous PR and focussed more on the design of the examples.

I debugged the scripts in the test and can indeed see the unintended script as described in this comment: https://github.com/bitcoin/bitcoin/pull/32114#discussion_r2034051063

```
('encodeable_pushdata1', CScript([OP_DROP, x('96616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616161616
...

💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054706420)
Shouldn't this one be called `nonstd_2byte_pushdata1` because it uses the `PUSHDATA1` opcode?
That's what is mentioned in the comments of the previous PR and in the spender comment down below.

💬 rkrux commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054707416)
I'm guessing it's intentional to have two different ways of creating `CScript` here?
For better illustration.

💬 maflcko commented on pull request "test: Run all benchmarks in the sanity check":
(https://github.com/bitcoin/bitcoin/pull/32310#issuecomment-2822268104)
For reference, in the two CI `DEBUG=1` configurations, the bench part seems to take 3x the time. Though, this should be fine, given the motivation in the pull description, and also the fact that there are similarly slow other tests.

💬 TheCharlatan commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054728640)
The comment may be confusing in the context of just using nlocktime, so maybe it should be amended to describe its non-BIP68 meaning too?

💬 maflcko commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054735178)
e78e92f6363c4ea1cbedd6631fe56c0cd8e164d5: I don't think this is right. Why the `-1`?

For reference, I debugged this via:

```diff
diff --git a/src/test/fuzz/utxo_snapshot.cpp b/src/test/fuzz/utxo_snapshot.cpp
index 13f6088192..927fcb483c 100644
--- a/src/test/fuzz/utxo_snapshot.cpp
+++ b/src/test/fuzz/utxo_snapshot.cpp
@@ -92,6 +92,7 @@ void initialize_chain()
Assert(processed);
const auto* index{WITH_LOCK(::cs_main, return chainman.m_blockman.LookupBlockInd
...

💬 maflcko commented on pull request "miner: timelock the coinbase to the mined block's height":
(https://github.com/bitcoin/bitcoin/pull/32155#discussion_r2054746098)
Yeah, the comments here are one-line summaries of the "real" comments in `./src/primitives/transaction.h` only. A third alternative may be to just remove them and refer to the docs in `./src/primitives/transaction.h` to avoid having to copy paste the full docs into both places every time they are touched.

🤔 BrandonOdiwuor reviewed a pull request: "test: Run all benchmarks in the sanity check"
(https://github.com/bitcoin/bitcoin/pull/32310#pullrequestreview-2785224059)
Code Review ACK faca46b0421b568e7e5fefe593420e773d0ec9af

💬 instagibbs commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054795871)
The python framework won't let you construct a non-standard serialization natively, AFAIK. So we're doing it manually.

💬 instagibbs commented on pull request "test: fix pushdata scripts":
(https://github.com/bitcoin/bitcoin/pull/32270#discussion_r2054797532)
I was focusing on two different ways of pushing 2 bytes, not naming the specific opcode. If it would help legibility I can re-introduce the exact pushdata opcode name in use.

💬 mzumsande commented on pull request "Fix tiebreak when loading blocks from disk (and add tests for comparing chain ties)":
(https://github.com/bitcoin/bitcoin/pull/29640#discussion_r2054831851)
Is this assertion correct - why can't the best blockhash be B8?
Even though B7 is received first, both are put into `std::multimap<CBlockIndex*, CBlockIndex*> m_blocks_unlinked;` (with no comparator, so pointer address is used which is decided by the OS?!). and later, when their parent arrives, accessed via `equal_range` in `ReceivedBlockTransactions` where `nSequenceId` is then set in that order. Couldn't this be done in the reverse order B8 -> B7 if the OS gives out the pointer addresses in a
...

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054848643)
I agree this is pretty intricate. I'll try to explain in the comments, to see if you have any suggestions for making it clearer.

The `SetType modified;` variable contains a conservative *overestimate* of which clusters in the real graph may differ between main and staging. Whenever a transaction is added, or removed, or undergoes a dependency adding in the simulated staging graph, it is definitely considered modified. But for example, say transactions A->B exists in main & staging, so they're
...

💬 janb84 commented on pull request "depends: Fix cross-compiling on macOS":
(https://github.com/bitcoin/bitcoin/pull/32215#issuecomment-2822454186)
> > I'm trying to test this PR, but having trouble verifying it. Am I correct to conclude that the fix is really mac only ( not nix-shell on macos? or GUIX cross-compile ? )
> > Can confirm that the `mv -t` option is not supported on MacOS .
>
> This PR is specific to cases like #32208.

Have installed homebrew and have successfully recreated the error as #32208.
This commit will resolve the error from #32208 but I do get an different error back, not sure if related to `Specifies Objecti
...

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054855051)
Done.

💬 sipa commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054855288)
Leftover from a previous change. Fixed!

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#issuecomment-2822521897)
ACK a0becaaa9c7390bc80d5864f7fffb32e21502a50

Added assertion, cleaned up comment, and rebase on master

`git range-diff master d64bd31f9b6903f0a335a78b519d0b52e84cca1b a0becaaa9c7390bc80d5864f7fffb32e21502a50`

💬 ismaelsadeeq commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2054912979)
This explanation clarified the comment, thanks.