💬 MarcoFalke commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#issuecomment-1681873254)
rebased

💬 MarcoFalke commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1681881324)
Will squash if there is conceptual agreement (Not sure when this concept last came up, but I think @dergoegge and @achow101 mentioned it?). Review question: Looks like the worst-case runtime is 1 hour per commit, so with GHA at most 6 commits could be tested before a timeout. Maybe a self-hosted worker with a longer timeout would be better?

💬 dergoegge commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1681892651)
Concept ACK

> Maybe a self-hosted worker with a longer timeout would be better?

Probably but I am wondering a bit if these task could end up clogging our self-hosted worker queue and hinder other jobs from running in their regular time?

🤔 vasild reviewed a pull request: "Improves addnode / m_added_nodes logic"
(https://github.com/bitcoin/bitcoin/pull/28155#pullrequestreview-1582006631)
Approach ACK a6fcf17f9dde56637bcfe824bb3def83e764b38c

💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296841654)
In the commit message of 12e601c33e `rpc: Prevents adding the same ip more than once when formatted differently`:

> ... formatting IPs in aborts
different,

💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296821862)
nit, style (`const` and attach the `&` to `auto`):
```suggestion
for (const auto& r : resolved) {
```

💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296827360)
This log can be improved to something like this:

```
Not opening a new connection to pool.bitcoinnodes.org, already connected to 1.2.3.4

💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296882332)
nit: use `resolved.IsValid()` instead of `resolved != CService{}`.

💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296888861)
```suggestion
std::vector<AddedNodeInfo> vInfo = GetAddedNodeInfo(/*include_connected=*/false);
```
and in other call sites too

💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296885786)
style (same in the declaration):
```suggestion
std::vector<AddedNodeInfo> CConnman::GetAddedNodeInfo(bool include_connected) const
```

💬 vasild commented on pull request "Improves addnode / m_added_nodes logic":
(https://github.com/bitcoin/bitcoin/pull/28155#discussion_r1296832854)
The old code would pick a random address from the list of resolved addresses. The new code would pick the last one. I think it is good to preserve the randomness. One way to achieve that would be to shuffle the result before iterating and picking the last one.

```cpp
if (!resolved.empty()) {
Shuffle(resolved.begin(), resolved.end(), FastRandomContext{});
for (const auto& r : resolved) {
```

or if the shuffle is perceived expensive for big lists, before the loop `auto to_pick =
...

💬 MarcoFalke commented on pull request "ci: Add test-each-commit task":
(https://github.com/bitcoin/bitcoin/pull/28279#issuecomment-1681905132)
> Probably but I am wondering a bit if these task could end up clogging our self-hosted worker queue and hinder other jobs from running in their regular time?

Right, that is another reason *for* self-hosted runners, because it is possible to assign a label to them, so that they are task-specific, and not interfere with other tasks. On GHA "free" runners will stop being spun up after ~n total running tasks.

💬 vasild commented on issue "p2p sends local-scope addresses when IPv4 is behind NAT":
(https://github.com/bitcoin/bitcoin/issues/8616#issuecomment-1681907760)
https://github.com/bitcoin/bitcoin/pull/27411 should have resolved 3.

💬 dergoegge commented on pull request "fuzz: call `LookupSubNet` before calling `Ban` with a subnet":
(https://github.com/bitcoin/bitcoin/pull/27935#issuecomment-1681918508)
This still crashes for me on the following input `/r8BAAAC/9LcAF8gMiWADQUA//////////////8AN1wNXMnJyQ3///8Afv///wABAAAAgAAAAADPvg==`

💬 darosior commented on pull request "fuzz: a target for the block index database":
(https://github.com/bitcoin/bitcoin/pull/28209#issuecomment-1681920298)
Rebased on master to fix the macOS CI.

💬 darosior commented on pull request "fuzz: a new target for the coins database":
(https://github.com/bitcoin/bitcoin/pull/28216#issuecomment-1681922742)
Rebased on master to fix the macOS CI.

💬 MarcoFalke commented on pull request "validation: don't clear cache on periodic flush":
(https://github.com/bitcoin/bitcoin/pull/28233#issuecomment-1681928913)
Not sure what's up with CI. Maybe rebase?

💬 MarcoFalke commented on pull request "p2p: Fill reconciliation sets and request reconciliation (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/26283#issuecomment-1681931026)
Looks like the functional test times out in CI?

💬 MarcoFalke commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1681932118)
Could rebase for CI, if still relevant?

💬 MarcoFalke commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#issuecomment-1681933926)
Maybe mark as draft while CI is red and this is based on other pulls?