💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913754168)
`#include <map>` and a few others can likely be removed now

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913774345)
Yes there is one edge-case there where it is necessary for it to be optional, unfortunately, and no length is directlly available.
i guess we could also make it non-optional then do something else at the call-site there (e.g. create a `AddrLenFromFamily` function or such), fine with me too.

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2588142467)
> PROTOCOL_ERROR is the only variant of MappingError which is not covered by the unit test. If you feel that's worth including it i wrote https://github.com/darosior/bitcoin/commit/be7482c2f6f754cfa4c205cff66ba9c689d426f5 to cover the 3 code paths in which it occurs.

Thank you! Will pull that in.

📝 maflcko opened a pull request: " refactor: Enforce readability-avoid-const-params-in-decls "
(https://github.com/bitcoin/bitcoin/pull/31650)
Top level `const` in declarations is problematic for many reasons:

* It is often a typo, where one wanted to denote a const reference. For example `bool PSBTInputSignedAndVerified(const PartiallySignedTransaction psbt, ...` is missing the `&`. This will create a redundant copy of the value.
* In constructors it prevents move construction.
* It can incorrectly imply some data is const, like in `std::span<int> Shuffle(const std::span<int>);`, where the `int`s are not const.
* The compiler ig
...

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913788476)
This doesn't work, boost apparently does derive from it.
```
.../src/test/pcp_tests.cpp:260:22: error: cannot derive from ‘final’ base ‘PCPTestingSetup’ in derived type ‘pcp_tests::natpmp_ipv4’
260 | BOOST_AUTO_TEST_CASE(natpmp_ipv4)
| ^~~~~~~~~~~
.../src/test/pcp_tests.cpp:311:22: error: cannot derive from ‘final’ base ‘PCPTestingSetup’ in derived type ‘pcp_tests::pcp_ipv4’
311 | BOOST_AUTO_TEST_CASE(pcp_ipv4)
| ^~~~~~~~
```

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913790664)
Done. It's actually the injected error, in case send/receive has to fail.

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913790833)
Done

🤔 furszy reviewed a pull request: "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor"
(https://github.com/bitcoin/bitcoin/pull/31590#pullrequestreview-2547867476)
ACK c0045e6cee06bc0029fb79b5a531aa1f2b817424

💬 laanwj commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913795001)
Ok, made it non-optional now. Somehow feels more consistent. It's likely that this PR will go in first anyhow. Will look into a different solution for #31014.

💬 hodlinator commented on pull request "optimization: batch XOR operations 12% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2588220255)
Nodes on local LAN, same commits, both on SSD. Syncing node was laptop running 13th Gen Intel i7, 20 logical cores.

#### Full node / source

Made to not have any other connections (verified through running with `-debug=net` for a while).

Deleted *anchors.dat* & *peers.dat*.

```
₿ build/src/bitcoind -dbcache=30000 -nofixedseeds -nodnsseed
```

#### Syncing node

Deleted *~/.bitcoin*.

```
₿ time build/src/bitcoind -dbcache=30000 -stopatheight=878000 -connect=<sourcenode>
```
...

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#issuecomment-2588243197)
re-ACK 9cd85281c6cc8d3fb01930e53109fae97a108dd8

👍 laanwj approved a pull request: "doc: Archive 28.1 release notes"
(https://github.com/bitcoin/bitcoin/pull/31630#pullrequestreview-2547939213)
ACK bb5f76ee013ee439f70e86319d22f308db8a24ea

💬 davidgumberg commented on pull request "descriptors: Try pubkeys of both evenness when retrieving the private keys for an xonly pubkey in a descriptor":
(https://github.com/bitcoin/bitcoin/pull/31590#issuecomment-2588255783)
utACK https://github.com/bitcoin/bitcoin/pull/31590/commits/c0045e6cee06bc0029fb79b5a531aa1f2b817424

I think this fixes #31589, and fixes a bug in miniscript parsing of xonly pubkeys. [^1]

[^1]: I think there might still be an issue with `ConstPubkeyProvider::GetSize()`, I'll try to look into this and open a follow-up if necessary.

👍 hodlinator approved a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2546259926)
ACK eb325bc0efd3f999189e155ba836be92e6cc9af7

Nice break up of commits for correcting each `-arg`.

In my case I still reviewed mostly through:
```
₿ git diff 2eccb8bc5e2245e42e05b8df3d9aa264b5f21a9a 80608ba5d282ae8713ea0136ea9a0208b254c082 > old
₿ git diff 2eccb8bc5e2245e42e05b8df3d9aa264b5f21a9a eb325bc0efd3f999189e155ba836be92e6cc9af7 > new
₿ meld old new &
```

*feature_config_args.py* - Nice that we are no longer relying on "leaked" `connect_arg` from prior `for`-loop, instead m
...

💬 hodlinator commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1912983327)
Typo
```suggestion
However, there are exceptions to this general rule. For example, it is an error to negate some options (e.g. `-nodatadir` is disallowed), and some negated strings are treated like `"0"` instead of `""` (e.g. `-noproxy` is treated like `-proxy=0`), and negating some lists can have side effects in addition to clearing the lists (e.g. `-noconnect` disables automatic connections in addition to dropping any manual connections specified previously with `-connect=<host>`). When the
...

📝 maflcko opened a pull request: "ci: Turn CentOS task into native one"
(https://github.com/bitcoin/bitcoin/pull/31651)
Cross-compiling to `i686-pc-linux-gnu` on CentOS in the CI is mostly redundant with the `ci/test/00_setup_env_i686_multiprocess.sh` task (albeit it using clang):

https://github.com/bitcoin/bitcoin/blob/35bf426e02210c1bbb04926f4ca2e0285fbfcd11/ci/test/00_setup_env_i686_multiprocess.sh#L9-L12

One task seems sufficient as a sanity check, given that there seems to be no real demand for this architecture anyway.

Turning the task into a native one makes it easier to run it on aarch64 or any o
...

💬 adamandrews1 commented on pull request "rpc: combinerawtransaction now rejects unmergeable transactions":
(https://github.com/bitcoin/bitcoin/pull/31298#discussion_r1913911212)
This is done to align the method with PSBT. It could be removed without breaking the change. But it also is just clearing data which should take minimal time. I think it's worth keeping to align with PSBT format.

🤔 mzumsande reviewed a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2548206643)
Code Review ACK 66dbfaca22ba55aea3a5e7ab73974b42134ba8f4

💬 mzumsande commented on pull request "test: avoid internet traffic":
(https://github.com/bitcoin/bitcoin/pull/31646#discussion_r1913933802)
could use `UNREACHABLE_PROXY_ARG` here as well

👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2548335954)
code review re ACK b945668216abbc978ce351b8a89910e651c6e595

Changes were to address the latest comments (removing "0x" and adjusting a comment).

```diff
$ git range-diff 228aba2c4d9ac0b2ca3edd3c2cdf0a92e55f669b..58b828f7f7ad216c7d7bf2e2ff431a66591e9d5c e8ea483fec05d824d2f35182b1a3bd68416b2df3~..b945668216abbc978ce351b8a89910e651c6e595
1: 6accee1844 = 1: e8ea483fec consensus: add DeriveTarget() to pow.h
2: 3e976d4171 = 2: 6a72497f50 build: move pow and chain to bitcoin_common
3
...