💬 MarcoFalke commented on pull request "move-only: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1514862798)
re-ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6 🚲

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK be55f545d53d44fdcf2
...

💬 MarcoFalke commented on pull request "move-only: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#issuecomment-1514870994)
unrelated: red CI can be ignored

💬 MarcoFalke commented on pull request "blockstorage: Adjust fastprune limit if block exceeds blockfile size":
(https://github.com/bitcoin/bitcoin/pull/27191#issuecomment-1514877737)
unrelated: Red CI can be ignored, or if you care a lot, you can rebase

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1171479645)
The unit tests are what I run locally over and over, as they run so much more quickly. IIRC there was a project idea a few years ago to convert the functional tests in Python to C++. Having better unit test coverage, or shifting coverage from the Python tests to C++ ones, seems beneficial.

💬 aureleoules commented on pull request "init: Add option for rpccookie permissions":
(https://github.com/bitcoin/bitcoin/pull/26088#issuecomment-1514901152)
I don't have much time to work on Core so feel free to pick this up @willcl-ark.

💬 fanquake commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#issuecomment-1514902450)
> is DEBUG=1 enabled for them as well in depends?
> Edit: I guess that would make them unusably slow?

They don't run too slow, but they seem to find bugs. #27222 reproduces on `x86_64`:
```bash
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-04-19T14:50:06Z Seed: Setting random seed for current tests to RANDOM_CTX_SEED=bcbaed399ecb4b53cf5998534b1cb7a9bdddd1c1755a5766212f5af3
...

💬 fanquake commented on issue "test: use-of-uninitialized-value in sqlite3Strlen30":
(https://github.com/bitcoin/bitcoin/issues/27222#issuecomment-1514903784)
See #27448. I've reproduced this (locally).

💬 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)