💬 0xB10C commented on pull request "test: create deterministic addrman in the functional tests":
(https://github.com/bitcoin/bitcoin/pull/29007#discussion_r1418567733)
We're doing 1. in the [feature_addrman.py](https://github.com/bitcoin/bitcoin/blob/2e8ec6b338a825a7155fff1be83993e3834ab655/test/functional/feature_addrman.py#L160-L161) functional test.

💬 maflcko commented on pull request "build: switch to using LLVM 17.x for macOS builds":
(https://github.com/bitcoin/bitcoin/pull/28880#issuecomment-1844894888)
> or in the combination of GCC 11 & LLVM 17

Tried testing this on Jammy, but it seemed fine, or maybe I did something wrong?

```
# riscv64-linux-gnu-g++ --version
riscv64-linux-gnu-g++ (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.


# ninja -C build dsymutil
ninja: Entering directory `buil
...

💬 S3RK commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1844899295)
Repeated my manual tests, all works.

The only thing I want to clarify is the downgrade + encrypt situation. So now the wallet loads and not "corrupted" but if the user would generate new descriptors (as proposed in future PRs) the wallet will use the pre-encryption HD key. The scenario seems pretty rare, but still slightly concerning as the pre-encryption HD key could've been compromised.

Maybe we should consider only loading key from active descriptors? In this case the wallet will "loose
...

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#issuecomment-1844906757)
Took a minor suggestion from @sr-gi, fixed clang-formatting, fixed code distribution between the two last commits.

⚠️ maflcko opened an issue: "fuzz: Fix stability, determinism issues"
(https://github.com/bitcoin/bitcoin/issues/29018)
It would be good to track fuzz "stability" and determinism, and fix any issues.

Is there an easy way to generate a table for this metric for each fuzz target, maybe as a side effect of CI, or in another way?

cc @dergoegge

💬 maflcko commented on pull request "wip: Split fuzz binary":
(https://github.com/bitcoin/bitcoin/pull/29010#issuecomment-1844917023)
I guess the "one fuzz target per file" makes the "one fuzz binary per message/rpc type" idea a bit harder to implement: https://github.com/bitcoin/bitcoin/pull/28015#issuecomment-1621453405 ?

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418616411)
I won't mind, but this should be commented somewhere above — perhaps where 0 returned is handled.

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418629765)
6c65f61b625212e128e5db52cbd80a3b57b1450b

nit: your code assumes that N is less than the capacity of `size_t`, otherwise this addition can overflow. You might consider adding a static assert for that too, just in case someone in the future decides to use `TimeOffsets<bazillion>`

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418647036)
After your changes, we won't consider `BLOCK_RELAY`, `FEELER`, `ADDR_FETCH` anymore, right?

🤔 MarnixCroes reviewed a pull request: "Fix: Disable both "Mask Values" and "Transaction View" if no wallet is selected"
(https://github.com/bitcoin-core/gui/pull/780#pullrequestreview-1769626322)
nice find

I'm not sure if this is the right approach: user might want to enable `Mask values` before opening his wallet?

💬 Sjors commented on pull request "datacarriersize: Match more datacarrying":
(https://github.com/bitcoin/bitcoin/pull/28408#issuecomment-1844981799)
This does not meet the bar for "good". If only Ocean pool uses this and it remains a relatively small pool, it will have no effect. If it is widely deployed, it's trivial to circumvent, which will cause this code to grow in complexity. And every time that happens there'll be a lot of pressure on maintainers to push it through quickly "to stop the spam".

For example you're currently looking for this pattern, used for inscriptions:

```
OP_FALSE OP_IF spam OP_ENDIF
```

This pattern fits
...

📝 TheCharlatan opened a pull request: "mempool: Don't sort in entryAll"
(https://github.com/bitcoin/bitcoin/pull/29019)
Current call sites of entryAll do not require the entries to be sorted, so iterate through mapTx directly to retrieve the entries instead of using SortedDepthAndScore.

This is a follow up to #28391 and was noted here https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418261403.

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418654682)
Why don't you implement going out of this warning?

💬 TheCharlatan commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1418656096)
This function is supposed to mimic `infoAll`, which was originally used in this patchset to retrieve mempool information instead of directly accessing `mapTx`. Looking at its usage you are right, the way I read this too there were no order promises made prior to this patch. I opened https://github.com/bitcoin/bitcoin/pull/29019 to instead just iterate through `mapTx`.

💬 naumenkogs commented on pull request "Nuke adjusted time (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#discussion_r1418656693)
>stops sampling entirely after 200 samples

perhaps record this bug in the commit message, might be useful in the future?

💬 maflcko commented on pull request "mempool: Don't sort in entryAll":
(https://github.com/bitcoin/bitcoin/pull/29019#issuecomment-1845000268)
lgtm ACK 87fa444a04854a5d3a9ff283a8b6c34587e8430f

Though, if the function will be removed either way, I wonder if this is worth it. Maybe wait for an OK by @sdaftuar to avoid a merge conflict with https://github.com/bitcoin/bitcoin/pull/28676 ?

✅ fanquake closed a pull request: "build: Require C++20 compiler"
(https://github.com/bitcoin/bitcoin/pull/28349)

🚀 fanquake merged a pull request: "build: use macOS 14 SDK (Xcode 15.0)"
(https://github.com/bitcoin/bitcoin/pull/28622)

📝 maflcko reopened a pull request: "build: Require C++20 compiler"
(https://github.com/bitcoin/bitcoin/pull/28349)
C++20 allows to write safer code, because it allows to enforce more stuff at compile time (`constinit`, `conteval`, `constexpr`, `std::span`, ...).

Also, it allows to write less verbose and easier to understand code (C++ 20 Concepts).

See https://github.com/bitcoin/bitcoin/issues/23363 and https://en.cppreference.com/w/cpp/compiler_support#cpp20

With g++-10 (https://github.com/bitcoin/bitcoin/pull/28348) and clang-13 (https://github.com/bitcoin/bitcoin/pull/28210), there is broad suppor
...

👋 maflcko's pull request is ready for review: "build: Require C++20 compiler"
(https://github.com/bitcoin/bitcoin/pull/28349)