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

💬 hebasto commented on pull request "depends: Fix cross-compiling on macOS":
(https://github.com/bitcoin/bitcoin/pull/32215#issuecomment-2822566913)
@janb84
> ... but I do get an different error back

Please provide more details about your build environment (OS version, architecture, compiler version etc) and exact steps to reproduce the error.

💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r2054951460)
Thanks! Done.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2054994737)
Yeah, that's right, fixed it.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r2054994837)
Fixed

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2822656585)
Rebased and addressed @sipa 's feedback. Also made a few further improvements: Added some missing includes, dropped some docs that have been added in #32110 already (silent merge conflict), unified asmap version/checksum naming (I went with version because that's what we have been communicating to users already).

> Would it be possible to separate these into two commits (probably first switching the interpretation to be bit-of-std::byte based but leaving everything in std::vector<std::byte>)
...

💬 Jokacar10 commented on issue "fuzz: psbt_base64_decode fails on Windows":
(https://github.com/bitcoin/bitcoin/issues/32135#issuecomment-2822672611)
assert(DecodeBase64PSBT(psbt, random_string, error) == error.empty());

💬 luke-jr commented on pull request "[IBD] prevector: store `P2WSH`/`P2TR`/`P2PK` scripts inline":
(https://github.com/bitcoin/bitcoin/pull/32279#discussion_r2055030024)
nit: why moving this?

💬 achow101 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-2822773252)
ACK 664657ed134365588914c2cf6a3975ce368a4f49

> passes as the bug does not manifest if "internal" is not provided by user

I see. Can you mention that in the PR description?

> re-based on current master

There's no need to rebase if there are no conflicts.

👍 ryanofsky approved a pull request: "fuzz: enable running fuzz test cases in Debug mode"
(https://github.com/bitcoin/bitcoin/pull/32113#pullrequestreview-2785668286)
Code review ACK 3669ecd4ccd8e7a1e2b1a9dcbe708c51c78e4d6c with just variable renamed and documentation added since last review