👍 furszy approved a pull request: "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC"
(https://github.com/bitcoin/bitcoin/pull/28118#pullrequestreview-1552328188)
Code review ACK fabef121. Convinced by checking all current tests usages.
Thanks Marko.

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655892517)
Imo these tests are buggy and are correctly failing here.
> test/logging_tests.cpp:86:77: error: Unterminated format string used with LogPrintf [bitcoin-unterminated-logprintf,-warnings-as-errors]
> LogPrintf_("fn1", "src1", 1, BCLog::LogFlags::NET, BCLog::Level::Debug, "foo1: %s", "bar1\n");

If we don't enforce that the newline is in the format-string, it's really nasty to enforce that it's "somewhere in one of the arguments", as that could even be added to a string at runtime.

As f
...

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277709470)
nit: It may be good to move this (and `export DIR_IWYU="${BASE_SCRATCH_DIR}/iwyu"`) to the only test that needs it. Otherwise it seems odd to export it to all test envs, even if they don't need it?

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277721719)
Yea. Refactored to remove `DIR_CORE_TIDY` entirely.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655915934)
Dropped `DIR_CORE_TIDY` and just used `BASE_SCRATCH_DIR` directly.
Added `example_logprintf.cpp` to the lint-format-strings.py exclusion list.

💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655918795)
> As far as I can tell the only place this is used is in these tests, so I'd suggest just deleting them so that we can enforce the rule sanely.

Actually since they test `LogPrintf_` directly, I suggest just fixing up the newline part so that we no longer support that construction.

```diff
diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
index 2699d316da..e448805e69 100644
--- a/src/test/logging_tests.cpp
+++ b/src/test/logging_tests.cpp
@@ -83,10 +83,10 @@ BOOST_AU
...

💬 stickies-v commented on pull request "ci: label docker images and prune dangling images selectively":
(https://github.com/bitcoin/bitcoin/pull/27793#issuecomment-1655919140)
Rebased to address merge conflict from https://github.com/bitcoin/bitcoin/pull/28138.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277725979)
If you don't do it first I'll cleanup `DIR_IWYU`.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277727111)
Nice. Feel free to clean it up here.

💬 sr-gi commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1655993325)
> would it make sense to add a functional test like the below under
>
> ```
> ip_port = "localhost:[]".format(p2p_port='add')
> self.nodes[0].addnode(node=ip_port, command='add')
> ```
>
> in `test/functional/rpc_net.py` on line 212 because we shortly after assert that there is only 1 added node added

I don't think that'd work. You'll still be able to add two nodes that resolve to the same IP using `addnode` as long as they belong to different namespaces. Lookups are not performed. T
...

🤔 stickies-v reviewed a pull request: "refactor: Add util::Result failure values, multiple error and warning messages"
(https://github.com/bitcoin/bitcoin/pull/25665#pullrequestreview-1243960352)
Will continue my review next week, this is mostly up until 332e847c9ec0241efd9681eee3b03ff819aaddc3

💬 stickies-v commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1277642391)
nit: would `m_error` or (`m_error_info`) be a more appropriate name? E.g. in `bool()`, the meaning of `!m_error` is much more intuitive at first sight compared to `!m_info`, I think? (i.e. it's not really clear what it means to "not have info", whereas "not have error" is clear).

```
explicit operator bool() const { return !m_info; }
```

💬 stickies-v commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1277588274)
nit: would it be more idiomatic to use `has_value()` here?
```suggestion
const T& value() const LIFETIMEBOUND { assert(has_value()); return m_value; }
T& value() LIFETIMEBOUND { assert(has_value()); return m_value; }
```

💬 stickies-v commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1277584643)
Would it make sense to rename this to `emplace`, to keep the interface in line with `optional` and `expected`?

💬 sr-gi commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#issuecomment-1656076583)
Added a test to `test/functional/rpc_net.py` and fixed the check on `CConnman::AddNode`

👍 jamesob approved a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1552660659)
reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic))

Changes incorporate feedback discussed above.


<details><summary>Show signature data</summary>
<p>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoi
...

💬 kevkevinpal commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1277945499)
might want to uncomment these other tests

💬 sr-gi commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1277960418)
Ups, my bad

💬 ariard commented on pull request "CONTRIBUTING: Caution against using AI/LLMs (ChatGPT, Copilot, etc)":
(https://github.com/bitcoin/bitcoin/pull/28175#issuecomment-1656240907)
Sent a mail to Jess from the Bitcoin Defense Legal Fund to collect more legal opinions with the context. Normally luke-jr (luke@dashjr.org) and jonatack (jon@atack.com) are cc.

💬 pinheadmz commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656279094)
> https://duckduckgo.com/?q=chattr+inside+docker should fix it

We're still having trouble getting this to work:
https://github.com/achow101/bitcoin/commits/28171-chattr-fixes
https://cirrus-ci.com/build/5095863295410176