💬 ariard commented on pull request "Add NODE_TXRELAY_V2.":
(https://github.com/bitcoin/bitcoin/pull/30837#issuecomment-2621950357)
@glozow Thanks for the comment.

Have a look on the original PR on #21224, when all the existing tests at the time were fixed, before a real conversation on the conceptual trade-offs that one can evaluate at high-level was effectively done. Sorry, not sorry, it’s not rational to fix CI if it’s to waste time again, before there is _conceptual convergence_.

As a contributor yourself, before to be a maintainer, and I think someone able to conceptually review PRs yourself, if there are technica
...

💬 glozow commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934088717)
Indeed not really prevented, just that it isn't trivially gameable.

💬 sipa commented on pull request "Add NODE_TXRELAY_V2.":
(https://github.com/bitcoin/bitcoin/pull/30837#issuecomment-2621959885)
@ariard Asking for conceptual feedback is reasonable, but here I don't even see any concept being proposed, neither in the PR nor in the associated BIP draft. It is *just* introducing a new flag, without any specification regarding what should be changed in the new protocol, nor even any discussion about what problems it is trying to address.

💬 ryanofsky commented on issue "build: depends cross-compile using Clang fails":
(https://github.com/bitcoin/bitcoin/issues/31748#issuecomment-2621962267)
> > You could also try CC=aarch64-linux-gnu-clang etc or whatever the aarch64 cross-compiler path is
>
> You don't really need to do this with, clang, it is natively a cross-compiler.

Yes that is what I meant by "compiler binaries usually have a default platform that they target".

Maybe I misunderstood context for your post and thought you were reporting a bug and looking for a workaround, not just reporting a bug. I think I agree that dropping CC=clang or specifying CC=aarch64-linux-gnu-clan
...

💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2621965610)
I'm now able to build depends with `make MULTIPROCES=1` on macOS without using the gnu versions.

However when using the result cmake complains about OpenSSL missing:

```
% cmake -B build --toolchain /Users/sjors/dev/bitcoin/depends/aarch64-apple-darwin24.3.0/toolchain.cmake
-- The CXX compiler identification is AppleClang 16.0.0.16000026
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /Applications/Xcode.app/Contents/Dev
...

💬 sipa commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934107086)
Should there perhaps be a preference for outbound peers here then, like in tx requesting?

💬 hebasto commented on issue "multiprocess: build failure on Alpine with depends & `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/issues/31455#issuecomment-2621983844)
> Does configuring with `-DCMAKE_BUILD_TYPE=Debug`, which pulls debug-specific macros, fix the issue?

It seems to [work](https://github.com/hebasto/bitcoin-core-nightly/actions/runs/13023723653/job/36329211464).

💬 mzumsande commented on pull request "test: fix intermittent timeout in p2p_1p1c_network.py":
(https://github.com/bitcoin/bitcoin/pull/31751#discussion_r1934137159)
done

👍 instagibbs approved a pull request: "test: fix intermittent timeout in p2p_1p1c_network.py"
(https://github.com/bitcoin/bitcoin/pull/31751#pullrequestreview-2581480055)
reACK 152a2dcdefa6ec744db5b106d5c0a8c5b8aca416

💬 fjahr commented on pull request "rpc: have getblocktemplate mintime account for timewarp":
(https://github.com/bitcoin/bitcoin/pull/31600#issuecomment-2622034950)
I didn't have time to write yesterday but I've been leaning towards having `mintime` honor the timewarp rule since it's more intuitive and it's inconsistent to say we want to prevent potential bugs but also maybe not all the bugs.

Did we have any blocks on mainnet in the last couple of years that would have violated the new `mintime`? If not, that should be a stronger argument that the `mintime` change is relatively safe. And if there are some maybe a compromise would be a deprecation cycle f
...

🤔 sipa reviewed a pull request: "Embed default ASMap as binary dump header file"
(https://github.com/bitcoin/bitcoin/pull/28792#pullrequestreview-2581421447)
Concept ACK.

I'd recommend integrating the last commit into the rest of the PR, making the `std::vector<bool>` -> `std::span<std::byte>` for the interpreter change first, and then introducing the embedded asmap data afterwards.

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1934116502)
This could be a *lot* smaller by using the `""_hex` user-defined literal (if you include `util/strencodings.h`); it'd create an `std::array<std::byte, N>` instead of a C-array, but the `std::span` constructor below just work just as well on it.

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1934103723)
This seems to be the prefix it's trying to match itself, not code for matching it.

I suggest: `static const auto = "fb03ec0fb03fc0fe00fb03ec0fb03fc0fe00fb03ec0fb0fffffeff"_hex_v;`

💬 sipa commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1934122382)
This new `gen_header` command doesn't seem to be used by the build system; what is its point?

💬 ariard commented on pull request "Add NODE_TXRELAY_V2.":
(https://github.com/bitcoin/bitcoin/pull/30837#issuecomment-2622044669)
@sipa Yeah now it’s post-embargo, I’ll update the 2 bips with more context, fix the code at minima for the 2 PRs, and prepare a list of questions on the trade-offs or comparison of different approaches, and then re-open a PR or ask on the ML for conceptual feedback. Should do it in the coming weeks.

🤔 marcofleon reviewed a pull request: "func test: Expand tx download preference tests"
(https://github.com/bitcoin/bitcoin/pull/31437#pullrequestreview-2581509816)
lgtm ACK 846a1387280fa584f70ccb1f5d0198339b065528

I commented out the delay addition for a non-preferred peer in `AddTxAnnouncement` and the inbound connection check in `test_preferred_inv` failed as expected.

Also did a simple change making all peers preferred when calling `ReceivedInv`. Removing that distinction causes `test_preferred_tiebreaker_inv` to fail (most of the time) because there's only a 1/11 chance `pref_peer` gets selected first.

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2622053406)
Working on a re-write of the 2 bips for this PR + #30837 and updating PRs now more context on the rational of the change is available: https://bitcoinops.org/en/newsletters/2024/12/06/

I should give an update in the coming weeks.

💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1934168830)
> I guess the check could be limited to exclude the msvc build?

Unfortunately, I can't find a simple way to detect whether binaries were built with MSVC without adding extra complexity to the Python code.

🤔 BrandonOdiwuor reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2581532869)
Concept ACK

💬 instagibbs commented on pull request "multi-peer orphan resolution followups":
(https://github.com/bitcoin/bitcoin/pull/31666#discussion_r1934173722)
The attack scenario here would be:
1) honest node gives you orphan_A
2) adversary (sybil) announce orphan_A
3) someone (could be adversary) gives parent_A, enters into mempool
4) node adds to adversary's workset
5) adversary disconnects
6) we don't re-evaluate the orphan

without this change, we may be doing the re-evaluation work up to num_inbound times erroneously?