💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2342376993)
For a followup or if I need to retouch.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3282694791)
> It would be helpful to mention why this is done in the commit message

If I need to retouch.

💬 achow101 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282710122)
ACK 653a9849d5f98ba80e334ddc0ae9a5e367459f59

🚀 achow101 merged a pull request: "common: Make arith_uint256 trivially copyable"
(https://github.com/bitcoin/bitcoin/pull/33332)

💬 davidgumberg commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282744720)
The members of `base_uint` are all [trivially copyable](https://en.cppreference.com/w/cpp/named_req/TriviallyCopyable.html): `WIDTH` is a scalar, and `pn` is an array of scalars, so the class is trivially copyable and we should use the implicit copy constructors of the class. But, by explicitly declaring the implicitly defined copy constructors the move constructors are suppressed.

<details><summary>

#### (https://en.cppreference.com/w/cpp/language/move_constructor.html)

</summary>


...

💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282754152)
if I understand this correctly, move constructors were suppressed before this PR as well.

💬 davidgumberg commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282762069)
> if I understand this correctly, move constructors were suppressed before this PR as well.

They were, so this is not a regression, but it would be better for the implicitly defined move constructors to be usable.

💬 Raimo33 commented on pull request "common: Make arith_uint256 trivially copyable":
(https://github.com/bitcoin/bitcoin/pull/33332#issuecomment-3282765661)

> it would be better for the implicitly defined move constructors to be usable.

good catch, totally agree, let's run some benchmarks

💬 achow101 commented on pull request "p2p: Advance pindexLastCommonBlock early after connecting blocks":
(https://github.com/bitcoin/bitcoin/pull/32180#issuecomment-3282783147)
ACK 3bdff9a154733f8f9f379448f5595a2e90474bc6

💬 achow101 commented on pull request "test: Fix CLI_MAX_ARG_SIZE issues":
(https://github.com/bitcoin/bitcoin/pull/33243#issuecomment-3282836726)
ACK fa96a4afea2a9bf90c843198e75a00acef02c32d

🚀 achow101 merged a pull request: "test: Fix CLI_MAX_ARG_SIZE issues"
(https://github.com/bitcoin/bitcoin/pull/33243)

💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3282884135)
I see that there were two failing checks -- is there any further suggested action I could take to help with this? I'm beginning to investigate now to try to understand what failed and why.

💬 achow101 commented on issue "RFC: Formal description of the RPC API":
(https://github.com/bitcoin/bitcoin/issues/29912#issuecomment-3282946065)
@laanwj @casey Are either of you still working on a PR for this?

💬 davidgumberg commented on pull request "cmake: fatal error when PIE not supported":
(https://github.com/bitcoin/bitcoin/pull/33282#issuecomment-3283024096)
> > I think the current behaviour of using PIE for all targets is sensible
>
> I am not convinced and I really would like to know the rationale for that. Maybe @hebasto or @theuni knows?

For motivation, you can look to the commit which enabled PIE by default: https://github.com/bitcoin/bitcoin/commit/3f94dfa25fc1b0e838d368a9b2683a634c.

In order for operating systems employing ASLR to randomize the base address of an executable's `text` segment, binaries must be built as PIE, and this be
...

🤔 w0xlt reviewed a pull request: "log: always print initial signature verification state"
(https://github.com/bitcoin/bitcoin/pull/33336#pullrequestreview-3214063456)
ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/39f41135c764c6c4705ce2cd36781ca8d43f0114

Simple test on mainnet:
```
./build/bin/bitcoind -datadir=new_data_dir/ -assumevalid=000000009b7262315dbf071787ad3656097b892abffd1f95a1a022f896f533fc -stopatheight=10
```

👍 davidgumberg approved a pull request: "test: fix p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3214061871)
ACK e14825ace2a8c6abcf11fdfb

- (https://github.com/bitcoin/bitcoin/pull/33121/commits/d73952df9e755f1349ff4a19c47ffc92efa3a2b1) Mitigates the likeliest cause of the spurious failure #33090 in `p2p_leak_tx.py` by increasing the timeout we'll wait for an INV.
- (https://github.com/bitcoin/bitcoin/pull/33121/commits/3005fb2ad39aec6fdb52dc6f406672a9515934ca) `test_notfound_on_unannounced_tx` previously was not really testing anything, as written in the PR description:
> we send a MSG_TX-t
...

💬 davidgumberg commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2342619001)
nanonit, only if retouching:

```suggestion
inbound_peer.wait_for_inv([CInv(t=MSG_WTX, h=wtxid)], timeout=120)
```

💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2342637583)
The rule of 5 just says if you define one, define/delete them all; I do it because that ensures I get a compile time error if there's some problem that makes one of them not possible. Move semantics probably aren't very interesting for a wrapper around an int though.

The C++ Core Guidelines version of the ["rule of 0"](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-zero) says not to define any of them when possible, however.

💬 ajtowns commented on pull request "Bump SCRIPT_VERIFY flags to 64 bit":
(https://github.com/bitcoin/bitcoin/pull/32998#discussion_r2342638488)
Note that it's consteval, so the throw is a compile-time check, not a runtime one.

💬 davidgumberg commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3283079195)
This CI failure is spurious, see https://github.com/bitcoin/bitcoin/issues/33345.

To force CI to rerun you can change your commit's timestamp (and hash) by doing `git commit --amend --no-edit` and push it.

💬 ajtowns commented on pull request "validation: ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r2342658393)
The minimumchainwork [should match](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/f36fa930bb99e332f2ccb9f76a42ea721850feb8/release-prep.sh#L151-L160) the assumevalid block, so if minimumchainwork isn't reached the assumevalid block won't be present and this code block won't be executed.

So this is only relevant for reindexing when you're manually choosing an older assumevalid block than the default, and aren't also adjusting the minchainwork. (If you choose a more recent assum
...