💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1514904243)
There may be a silent merge conflict. After rebase to current master at fde224a6610699a6a28cc27e299ac14cbf7e16ca, building with Clang 16 fails at commit `test: add a mocked Sock that allows inspecting what has been Send() to it`.

<details><summary>build error</summary><p>

```
test/util/net.cpp:280:12: error: no viable conversion from returned value of type 'const CNetMessage' to function return type 'std::optional<CNetMessage>'
return msg;
^~~
/opt/homebrew/opt/llvm/bin/
...

💬 ajtowns commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1514909267)
> Also, mempools _are_ full on mainnet

My mempool isn't full, and (I think) hasn't been full this year. (It has a 1GB limit; though there's only ~40MB of txs now anyway)

Submitting a low-feerate tx with a high mempool limit already means your tx won't relay without a cpfp tx, but we expect people to be who are clever enough to do that to also deal with the consequences. (If you do cpfp the low feerate tx, it'll be broadcast via orphan handling, so won't appear in the unbroadcast set any lo
...

💬 dergoegge commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1514913010)
> There may be a silent merge conflict.

Probably because of https://github.com/bitcoin/bitcoin/pull/27324, copying `CNetMessage`s is no longer possible.

⚠️ MarcoFalke reopened an issue: "test: use-of-uninitialized-value in sqlite3Strlen30"
(https://github.com/bitcoin/bitcoin/issues/27222)
https://cirrus-ci.com/task/5021971277152256?logs=ci#L3656

```
wallet/test/feebumper_tests.cpp(18): Entering test suite "feebumper_tests"
wallet/test/feebumper_tests.cpp(42): Entering test case "external_max_weight_test"
2023-01-30T17:19:34Z Seed: Setting random seed for current tests to RANDOM_CTX_SEED=231d587d0169ecab6befbed75f49c95aa84567b2750479dca13bd7471f2627e2
2023-01-30T17:19:34.255341Z [test] [init/common.cpp:149] [LogPackageVersion] Bitcoin Core version v24.99.0-a55717c914f8 (rel
...

💬 MarcoFalke commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514926359)
Steps to reproduce on a fresh install of Fedora 38:

* `dnf install gcc-c++ libtool make autoconf automake python3 clang llvm lbzip2 patch xz cmake curl wget htop git vim ccache libevent-devel boost-devel -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./bitcoin-core && cd bitcoin-core`
* `./autogen.sh && ./configure CC=clang CXX=clang++ --with-sanitizers=thread && make -j $(nproc)`
* `TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=
...

💬 MarcoFalke commented on issue "Replace all of the RecursiveMutex instances with the Mutex ones":
(https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514928912)
<details><summary>tsan</summary>

```
node0 stderr ==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=13857)
Cycle in lock order graph: M0 (0x7b6400005068) => M1 (0x7b440001ffa8) => M2 (0x7b5400003ee0) => M0

Mutex M1 acquired here while holding mutex M0 in thread T10:
#0 pthread_mutex_lock <null> (bitcoind+0x1a91af) (BuildId: 1db3d87bed913bc0e4da6baf5872efb9abf647be)
#1 __gthread_mutex_lock(pthread_mutex_t*) /usr/bin/../lib/gcc/x86_
...

⚠️ ajtowns opened an issue: "Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/issues/27494)
### Please describe the feature you'd like to see added.

From https://github.com/bitcoin/bitcoin/pull/27446#issuecomment-1513814497:

> This brings up a related issue in that we can't have multiple signets in the same base datadir. All signets use the signet datadir, so any change of parameter may result in a bad chainstate for that signet node.
>
> I think we should instead look at implementing a more robust multiple signet solution that allows for multiple configurable consensus rules in
...

💬 ajtowns commented on issue "Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/issues/27494#issuecomment-1514945137)
cc @achow101 @kallewoof

An option might be to name the chain dir `signet-XXXXXXXX` instead of `signet` where the `XX`s are the `pchMessageStart`, or perhaps to have ~40 `XX`s instead of 8 and have them be the hash160 of the signet challenge?

💬 MarcoFalke commented on issue "Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/issues/27494#issuecomment-1514949665)
This looks like a breaking change, so one could carve out the current global default signet to keep the current folder name. Though, for other signets this breaking change seems fine.

💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171528095)
Forgot to set this for the fuzz msan task?

👍 ryanofsky approved a pull request: "move-only: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1392362907)
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.

Confirmed move-only except for making InterpretValue function nonstatic, and moving some symbols from util to common namespace.

💬 ryanofsky commented on pull request "move-only: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1171524879)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (be55f545d53d44fdcf2d8ae802e9eae551d120c6)

Not important, but there is still some extra whitespace on this line

💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171530477)
And given that the CI fails here, maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?

🤔 pablomartin4btc reviewed a pull request: "blockstorage: do not flush block to disk if it is already there"
(https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1392374264)
Concept ACK. (went thru notes and other related PRs mentioned on the [Review club meeting preparation webpage](https://bitcoincore.reviews/27039) - Please add label when possiblel)

💬 fanquake commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171548431)
> Forgot to set this for the fuzz msan task?

I didn't bother as that isn't run here, and wouldn't compile rn (master) in any case.

> maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?

I think I'd rather defer to adding both, at the same time, in a follow up, if you're alright with that. Can drop the test commit from this PR.

🤔 darosior reviewed a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1392406641)
I really like the performance gains here, however i feel like this is making the spks management bleed into the wallet from the spkman. But there is no other way for the wallet to efficiently locate the spkman(s)..

I was looking into how to get rid of the callbacks and avoid duplicating the scriptpubkeys, and thought we might as well share a single mapping owned by `CWallet` across all spkmans. This would also have the nice property that we don't need to make sure moving forward we don't intr
...

💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#issuecomment-1514991825)
lgtm ACK 4de9c2a65f6770405f167c7392cd8371111bc4e8

🤔 jamesob reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1392348713)
Thanks to all the dedicated reviewers and my apologies for the delay. Lot of good feedback addressed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171527318)
Great catch, done.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171531164)
Fixed.