💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1901969607)
This does not need to be optional.

💬 darosior commented on pull request "test: Add mockable steady clock, tests for PCP and NATPMP implementations":
(https://github.com/bitcoin/bitcoin/pull/31022#discussion_r1913713048)
Ah, maybe it does for #31014.

💬 darosior commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2588044409)
Concept ACK.

💬 darosior commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913751718)
Seems worth keeping, at least for a few versions?

🤔 l0rinc requested changes to a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2547740151)
Concept ACK
The remaining "checkpoint" references seem unrelated.

💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913752692)
usually there's a single line per arg here
```suggestion
argsman.AddArg("-checkpoints", "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN); // Checkpoints were removed. We keep `-checkpoints` as a hidden arg to display a more user friendly error when set.
```

💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913746337)
you can likely remove `#include <ranges>` now

💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913753340)
this is already obvious from the error, the comment is redundant

💬 l0rinc commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1913741821)
To guard against `BLOCK_HEADER_LOW_WORK` changing its numeric value now (to 8 instead of previous 9), we could hard-code the previous value to the remaining ones
```C++
enum class BlockValidationResult {
BLOCK_RESULT_UNSET = 0, //!< initial value. Block has not yet been rejected
BLOCK_CONSENSUS = 1, //!< invalid by consensus rules (excluding any below reasons)
BLOCK_CACHED_INVALID = 2, //!< this block was cached as being invalid and we didn't store the reason why

...

💬 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