💬 maflcko commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060698390)
> The person introducing it has to check, once, that the tarball matches exactly what is in git (and reviewers can verify this)

I think it could make sense to specifically mention this on bumps, going forward. Maybe there could even be a new target written in depends to do the check?

💬 maflcko commented on pull request "[RFC] guix: remove xz from deps":
(https://github.com/bitcoin/bitcoin/pull/29895#issuecomment-2060707521)
> If anything, I'd hope it's less easy to backdoor, because it's just the source code, and not all the bootstrapped garbage we don't need. This is using the tag from the official upstream source control.

Oh, I see. So the motivation is to drop, possibly non-determinisically created, "bootstrapped garbage"? Concept +1 on this.

I am just not sure on dropping `$compression_tool`. I presume guix internally requires it to bootstrap to a point where Bitcoin Core can be compiled, as many guix sou
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2060724432)
Rebased. It no longer has a wallet dependency, which is nice.

> The RPC returns an array for each block with one entry per transaction. The entry is empty if there is no tweak. So the ordering is based on the order of transactions in a block.

The middle part here is wrong: when a transaction doesn't have a tweak it's simply skipped, there's no empty element. I could change that, but I don't think it's necessary for anything.

Here's the tweaks I get on a recent signet block:

<details>
...

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1568462751)
@josibake can you make `GetSilentPaymentTweakDataFromTxInputs` return a `CPub` (or `uint256`) rather than this low level secp thing?

🤔 glozow reviewed a pull request: "test: fix intermittent failure in p2p_compactblocks_hb.py"
(https://github.com/bitcoin/bitcoin/pull/29893#pullrequestreview-2005670115)
ACK 1ae5b208d339fa984d9caf4fab89b0b2ba9cc197

Thanks! I had this failure recently. The reason/fix makes sense to me and I tested that it works using the `sleep`.

✅ glozow closed an issue: "test: Intermittent issue in p2p_compactblocks_hb.py" in relay_block_through assert_equal(status_to, status_from) AssertionError: not([True, False, True, True] == [True, True, True, True])"
(https://github.com/bitcoin/bitcoin/issues/29860)

🚀 glozow merged a pull request: "test: fix intermittent failure in p2p_compactblocks_hb.py"
(https://github.com/bitcoin/bitcoin/pull/29893)

💬 fanquake commented on pull request "deploy: remove some tools when cross-compiling for macOS":
(https://github.com/bitcoin/bitcoin/pull/29890#issuecomment-2060960613)
Added a commit to decrease the scope of using `-Wl,-headerpad_max_install_names`.

💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2060962844)
> There are no conflicts with https://github.com/bitcoin/bitcoin/pull/29865.

Looks like there are now?

💬 laanwj commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2061002157)
Looks good to me now
Code review ACK 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86

💬 0xB10C commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2061008679)
> How is is possible that this even compiled? I presume tracing just takes any type and value and passes it along as raw bytes, so `std::chrono` being type-safe does not help here?
>
> Edit: If yes, one could consider if adding a static_assert to ban std::chrono (and possibly other types) from tracing?

In this case, casting with e.g. `int64_t{time_5 - time_start}` would have failed to compile too. Maybe being explicit with the type we expect here, as we do with e.g. the utxocache tracepoin
...

🤔 tdb3 reviewed a pull request: "ZMQ: Support UNIX domain sockets"
(https://github.com/bitcoin/bitcoin/pull/27679#pullrequestreview-2005766388)
cr re ACK. Changes lgtm. Will follow up with some testing within the next few days as time allows.

💬 maflcko commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2061024985)
lgtm ACK ca4eaa5777888292bbdef3ae8d45af6a407df5df

💬 maflcko commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2061028339)
> In this case, casting with e.g. `int64_t{time_5 - time_start}` would have failed to compile too. Maybe being explicit with the type we expect here, as we do with e.g. the utxocache tracepoints, is worth considering.

Another benefit would be that the code is self-documenting. If a comment for each argument is appended in-line, one could even generate the doc/tracing file from the source code, with the guarantee that it is accurate.

But that can be done in a follow-up.

🚀 fanquake merged a pull request: "guix: replace GCC unaligned VMOV patch with binutils patch"
(https://github.com/bitcoin/bitcoin/pull/29846)

💬 Christewart commented on pull request "tests: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2061048370)
@dgpv @petertodd what do you think is the path forward for this?

👍 alfonsoromanz approved a pull request: "doc: Add example of mixing private and public keys in descriptors"
(https://github.com/bitcoin/bitcoin/pull/28373#pullrequestreview-2005838311)
Re ACK 24b67fa9f602cdeac0e9736256f77d048f616c48

👍 alfonsoromanz approved a pull request: "test: Fix intermittent issue in p2p_handshake.py"
(https://github.com/bitcoin/bitcoin/pull/29898#pullrequestreview-2005867060)
Tested ACK b199b323840cf4978bda3437eee36c3dd228702a

I was able to reproduce this issue by using the sleep statement.

Test execution before this fix (failed):

<img width="743" alt="Screenshot 2024-04-17 at 08 58 30" src="https://github.com/bitcoin/bitcoin/assets/19962151/c0b2cf2a-b8f3-4ce0-8fc5-39788b563774">

Test execution after this fix (passed):

<img width="1061" alt="Screenshot 2024-04-17 at 09 05 43" src="https://github.com/bitcoin/bitcoin/assets/19962151/6e389d28-ddc4-4a9b-9c
...

💬 willcl-ark commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2061135454)
> BTW: along with some capstone magic like https://github.com/bitcoin/bitcoin/pull/29874, and a bare-bones DWARF (or linker map) parser to get function ranges, it would be possible to make a check like this that runs in the final binary, checking that special instructions only appear in allow-listed functions.

This sounds very cool (and more practically-useful than the check here perhaps).

> i think this script makes sense, it's good to check that objects with special instructions don't co
...

📝 glozow converted_to_draft a pull request: "26.x: backport #29853 ("sign: don't assume we are parsing a sane Miniscript")"
(https://github.com/bitcoin/bitcoin/pull/29854)
Backports #29853.