💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1655802652)
Pulled in the new changes, will update for exceptions shortly.

💬 mzumsande commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1277639934)
> why not `less than BLOCK_VALID_TRANSACTIONS`?

I asked myself the same question recently and came to the conclusion that it's because when we finally end up downloading the assumed-valid blocks, there will be a period in time where we've saved the block (and therefore raised its validity to `BLOCK_VALID_TRANSACTIONS`) but haven't connected it yet to the background chainstate (so it's still `ASSUMED_VALID`).

💬 furszy commented on pull request "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC":
(https://github.com/bitcoin/bitcoin/pull/28118#issuecomment-1655812661)
> That just seems brittle, and bloated, for no reason? It is brittle because one may miss to implement a proxy, as they'll have to be done for the bitcoin-cli utility and the python rpc wrapper, and devs can still accidentally sidestep it. It is bloated because it will be more than one line of code.

I'm not really strong here but It just seems a bit error-prone to always call to the `syncwithvalidationinterfacequeue` internally after bumping the scheduler timeout and not before. Existing call
...

💬 brunoerg commented on pull request "fuzz: add target for `ScriptPubKeyMan` (legacy)":
(https://github.com/bitcoin/bitcoin/pull/28153#discussion_r1277659822)
Yea, more elegant. Pushed changing it

💬 MarcoFalke commented on pull request "test: Add SyncWithValidationInterfaceQueue to mockscheduler RPC":
(https://github.com/bitcoin/bitcoin/pull/28118#issuecomment-1655832208)
> Existing callbacks using the node's time to operate could be affected and perform differently

No, they shouldn't be affected by this change. The scheduler thread is already free to behave at any time as-if `syncwithvalidationinterfacequeue` was just called.

This will only reduce intermittent test failure rates where the test assumed that the scheduler thread behaved as-if `syncwithvalidationinterfacequeue` was called and that it *must* happen within a specific time. As explained in the d
...

🚀 fanquake merged a pull request: "ci: Keep system env vars as-is (bugfix)"
(https://github.com/bitcoin/bitcoin/pull/28138)

👍 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`