💬 brunoerg commented on pull request "rpc: net: follow-ups for #30062":
(https://github.com/bitcoin/bitcoin/pull/30183#issuecomment-2148203825)
> Not sure the asmap feature cares if your map is based on BGP or something else?

AFAIK I think it doesn't care, but it's the expected. Which alternatives do you see?

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148229582)
> @hebasto Good idea, self copy-assignment was indeed broken! Fixed, and added tests for self copy/move/swap.

I suspect this is probably broken in several of our other classes as well. Speaking for myself at least, I almost always forget about correctness there and never look for it in review.

FWIW I tried using the [bugprone-unhandled-self-assignment](https://clang.llvm.org/extra/clang-tidy/checks/bugprone/unhandled-self-assignment.html) clang-tidy check which does catch the problem, but
...

💬 AngusP commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1626494800)
nit:

This `self.nodes[0].blocks_path / "blk..." / ...` is a [`Path`](https://docs.python.org/3/library/pathlib.html) so you don't need to `import os` to call `os.rename` to rename it, you can instead:

```suggestion
(self.nodes[0].blocks_path / "blk00000.dat").rename("blk00000.dat.copy")
```

Most of the things you could do to files/directories with `os` can also be done with `Path`.

(The `.rename` is aware of which bit of the path is the file so you don't need to give it the
...

💬 AngusP commented on pull request "test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure":
(https://github.com/bitcoin/bitcoin/pull/30174#discussion_r1626503734)
There's the "maximum 2-hour block timestamp difference" acceptance rule, but I don't think Valgrind is *that* slow 😆

👍 AngusP approved a pull request: "test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure"
(https://github.com/bitcoin/bitcoin/pull/30174#pullrequestreview-2097230263)
Untested ACK fa4f0decb9106b5046fda76baf2f7ed44a5d3c62

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148252096)
@theuni What if the comparison is changed to `if (this == &other) return *this;`?

🤔 instagibbs reviewed a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2096682721)
reviewed through 9b04e8864b2807c94a03b9093cfa8c836d7c7116, but I need to look closer at the fuzz cases

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626177967)
you can ignore this if you don't like it
```Suggestion
/** m_buffer + m_offset points to first object in queue. m_offset = 0 if m_capacity is 0; otherwise
```

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626321220)
Putting bounds on the capacity growth seems useful here and a few other spots, even if it doesn't necessarily have to be this tight.
```
if (old_cap > old_size) {
assert(real[idx].capacity() == old_cap);
} else {
assert(real[idx].capacity() > old_cap);
assert(real[idx].capacity() <= 2 * (old_cap + 1));
}
```

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626291871)
`s/Index/BufferIndex/` reduces my mental load fwiw

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626341902)
Calling `compare_fn` here seems more complete than picking and choosing when to call it before the final check
```Suggestion
if (!real.empty()) compare_fn(real[idx], sim[idx]);
if (non_full && command-- == 0) {
```

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626395824)
double checking: `first_part == 0` implies `m_size == 0`?

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626387016)
gave an attempt
```Suggestion
/** Returns the number of populated objects from m_offset to end of allocated memory. */
size_t FirstPart() const noexcept { return std::min(m_capacity - m_offset, m_size); }
```

💬 instagibbs commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626448996)
seems ever so slightly more direct, even though this seems correct as is
```Suggestion
if (&other == this) return *this;
```

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148290427)
> `if (this == &other) return *this;`

Huh, that worked. But this (the one I tried) doesn't
```c++
if (*this == other) return *this;
```

💬 theuni commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#issuecomment-2148299043)
Oh, duh, that's a totally different comparison. Ofc that didn't do what I wanted :)

💬 maflcko commented on pull request "test: Set mocktime in p2p_disconnect_ban.py to avoid intermittent test failure":
(https://github.com/bitcoin/bitcoin/pull/30174#discussion_r1626530422)
Yeah, I think it shouldn't matter here. An alternative would be to increase the magic number `120` sufficiently (maybe by a factor of 10 or so).

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1626544077)
Indeed!

💬 willcl-ark commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2148338315)
I think an upstream PR may be the best approach here, So I opened https://github.com/becheran/mlc/pull/89

💬 maflcko commented on pull request "ci: add markdown link check job":
(https://github.com/bitcoin/bitcoin/pull/30034#issuecomment-2148351046)
Not sure if globbing is the right fix. Conceptually, no folder or file should be scanned that is not tracked in git.

Though, I wonder if there is an easy way to ask git to print all files/folders that are "dirty", ignoring the gitignore. An alternative may be to teach mlc to read understand the gitignore files. However, that will also be incomplete, because devs may add random other folders themselves.

So for now the only solution would be to call `mlc` in a loop over `git ls-files -- '*.m
...