💬 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

🤔 Sjors reviewed a pull request: "Rework validation logic for assumeutxo"
(https://github.com/bitcoin/bitcoin/pull/27746#pullrequestreview-1552855193)
Also reviewed a733dd79e29068ad1e0532ac42a45188a040a7b9, d4a11abb1972b54f0babdccfbb2fde97ab885933, 3556b850221bc0e597d7dd749d4d47ab58dc8083 and read through all of `CheckBlockIndex()`. Will continue later (going mostly in reverse order).

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278023724)
3556b850221bc0e597d7dd749d4d47ab58dc8083: if you have to retouch, can you add a comment that `!CBlockIndexWorkComparator()(pindex, c->m_chain.Tip())` means "pindex has more work than the tip or was received earlier"

💬 Sjors commented on pull request "Rework validation logic for assumeutxo":
(https://github.com/bitcoin/bitcoin/pull/27746#discussion_r1278035016)
3556b850221bc0e597d7dd749d4d47ab58dc8083: there's a tie breaker in the comparator for if two blocks with equal work were loaded from disk. Presumably that only matters if you restart right during a reorg in some weird way.

💬 pinheadmz commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1656289233)
@naumenkogs I added one more commit that limits the number of simultaneous forced-inbound connections to 8. I didn't add it as an init option yet but we could if you like that approach. A connection only counts as forced-inbound if it succeeded in evicting another peer, and that is marked by a new bool in `CNodeOptions`

💬 achow101 commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1656327982)
ACK 19b6353d3866769ea84b7759396078745c0a8ecf

Although the `chattr` approach worked locally, we couldn't figure out how to make it work in CI, so the approach of running CI as a non-root user seems reasonable.

💬 achow101 commented on pull request "test: Add unit & functional test coverage for blockstore":
(https://github.com/bitcoin/bitcoin/pull/27850#discussion_r1278078208)
> It is an undocumented requirement that the functional usdt tests are run as root (not sure why).

For whatever reason, USDTs don't work for non-root users.