💬 naumenkogs commented on pull request "p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode":
(https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1427705233)
Concept ACK.
I think the 43% saving is a big deal, and the code change amount seems fine, although worth prototyping those corner cases first :)

W.r.t. the security reduction pointed out by @petertodd, I agree with the @JoseSK999 [comment](https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1426014422) almost entirely.
It could be emphasized further — 43% saving by itself helps the node-running culture as a whole (more than it hurts it by dropping witness checks).

I agree with @sd
...

💬 MarcoFalke commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1104282294)
Allowing for a `void` failure type seems to make this incompatible to be switched out to `std::expected` https://en.cppreference.com/w/cpp/utility/expected ?

With multiple warning and error messages this may already be incompatible, though?

💬 MarcoFalke commented on pull request "[WIP] p2p: Add random txn's from mempool to GETBLOCKTXN":
(https://github.com/bitcoin/bitcoin/pull/27086#issuecomment-1427723661)
Wouldn't it be better to not add wallet transactions to the mempool if we don't want peers to query our mempool for wallet transactions?

See also https://github.com/bitcoin/bitcoin/issues/11887#issuecomment-1061946262 (and all in- and out- links in this issue)

✳️ MarcoFalke pushed commits to a branch: bitcoin/bitcoin:master
(https://github.com/bitcoin/bitcoin/compare/b92d609fb256...a6316590d5bc)
Merge bitcoin/bitcoin#26970: test: fix immediate tx relay in wallet_groups.py

ab4efad51b9ba276ffeb6871931e13772493f7cc test: fix immediate tx relay in wallet_groups.py (Sebastian Falbesoner)

Pull request description:

In the functional test wallet_groups.py we whitelist peers on all nodes (`-whitelist=noban@127.0.0.1`) to enable immediate tx relay for fast mempool synchronization. However, considering that this setting only applies to inbound peers and the default test topology looks like this:
```
node0 <--- node1 <---- node2 <--- ... <-- nodeN
```

txs propagate fast only from lower- to higher-numbered nodes (i.e. "left to right" in the above diagram) and take long from higher- to lower-numbered nodes ("right to left") since in the latter direction we only have outbound peers, where the trickle relay is still active. As a consequence, if a tx is submitted from any node other than node0, the mempool synchronization can take quite long.

This PR fixes this by simply adding another connection from node0 to the last node, leading to a ~2-3x speedup (5 runs measured via `time ./test/functional/wallet_groups.py` are shown):

```
master:
0m53.31s real 0m08.22s user 0m05.60s system
0m32.85s real 0m07.44s user 0m04.08s system
0m46.40s real 0m09.18s user 0m04.23s system
0m46.96s real 0m11.10s user 0m05.74s system
0m57.23s real 0m10.53s user 0m05.59s system

PR:
0m19.64s real 0m09.58s user 0m05.50s system
0m18.05s real 0m07.77s user 0m04.03s system
0m18.99s real 0m07.90s user 0m04.25s system
0m17.49s real 0m07.56s user 0m03.92s system
0m18.11s real 0m07.74s user 0m03.88s system
```
Note that in most tests this is not a problem since txs very often originate from node0.

ACKs for top commit:
brunoerg:
utACK ab4efad51b9ba276ffeb6871931e13772493f7cc

Tree-SHA512: 12675357e6eb5a18383f2bfe719a184c0790863b37a98749d8e757dd5dc3a36212e16a81f0a192340c11b793eda00db359c7011f46f7c27e3a093af4f5b62147

🚀 MarcoFalke merged a pull request: "test: fix immediate tx relay in wallet_groups.py"
(https://github.com/bitcoin/bitcoin/pull/26970)

✳️ MarcoFalke pushed commits to a branch: bitcoin/bitcoin:master
(https://github.com/bitcoin/bitcoin/compare/a6316590d5bc...141115a06040)
Merge bitcoin/bitcoin#27033: ci: Cache stuff in volumes, not host folders

fa8e92c022057adcb8b98647bde626ed9c054df2 doc: Update ci docs (721217.xyz)
5fffff54e9fcf154c722dc421025a567fa0c5c97 ci: Cache stuff in volumes, not host folders (MarcoFalke)

Pull request description:

Storing cached stuff in host system folders may lead to unexpected issues when the ci-built stuff is used for a non-ci build or a ci task leaks into another ci task.

ACKs for top commit:
john-moffett:
ACK fa8e92c022057adcb8b98647bde626ed9c054df2

Tree-SHA512: 8b0c9019452fbe507a272c1037c3dce3c178c21f85ab1096ed3372ad9d4b3c7aa27d89e5bf80c9a6260ea652e0268be0cbe61d6a4fcb3add569fa38076d32287

🚀 MarcoFalke merged a pull request: "ci: Cache stuff in volumes, not host folders"
(https://github.com/bitcoin/bitcoin/pull/27033)

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104354933)
Thanks, I adapted these changes with @pinheadmz name change suggested below

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104355153)
Agreed, and renamed.

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104355315)
agreed

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104356262)
Now returns a file path

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1427800810)
Thanks @stickies-v & @pinheadmz , I addressed your comments in the new push.

💬 stickies-v commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104364182)
nit
```suggestion
const auto path{GetDataDir(net_specific)};
```

💬 stickies-v commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104368899)
```suggestion
args.EnsureDataDir(/*net_specific=*/false);
```

💬 stickies-v commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104369081)
```suggestion
gArgs.EnsureDataDir(/*net_specific=*/false);
```

💬 stickies-v commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104365397)
sorry, forgot to mark this as `const` in my diff
```suggestion
const auto conf_path{GetConfigFilePath()};
```

💬 hebasto commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#issuecomment-1427815930)
Concept ACK.

💬 willcl-ark commented on pull request "Convert ArgsManager::GetDataDir to a read-only function":
(https://github.com/bitcoin/bitcoin/pull/27073#discussion_r1104375173)
sure, also agree

👍 stickies-v approved a pull request: "refactor, wallet: Avoid variable shadowing"
(https://github.com/bitcoin/bitcoin/pull/27087)

📝 MarcoFalke opened a pull request: "ci: Use dedicated root path for ci "
(https://github.com/bitcoin/bitcoin/pull/27090)
Now that stuff (like depends) is no longer propagated back to the host
folder after commit 141115a0604001b8cce848804798dc174770a994, commit
85892f77c98c7a08834a06d52af3eb474275afd8 can be reverted and
`LOCAL_USER` removed.

So do that. Also for clarity, in the same commit, use a dedicated path
for the ci root folder `CI_ROOT_DIR`.