💬 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

💬 ryanofsky commented on pull request "fuzz: enable running fuzz test cases in Debug mode":
(https://github.com/bitcoin/bitcoin/pull/32113#discussion_r2055079547)
re: https://github.com/bitcoin/bitcoin/pull/32113

> Also, just because there may not be a performance difference today, doesn't mean there will be none in the future.

Am dubious of this. In general I'd think we'd want to have as few differences as possible between fuzzing and non-fuzzing code code paths and right now there is only 1 difference (outside of test code) in `CheckProofOfWork()`. Even if we were going to add more differences it's hard to imagine wanting to add them in hot code p
...

🚀 ryanofsky merged a pull request: "fuzz: enable running fuzz test cases in Debug mode"
(https://github.com/bitcoin/bitcoin/pull/32113)