👍 cbergqvist approved a pull request: "util: add BitSet"
(https://github.com/bitcoin/bitcoin/pull/30160#pullrequestreview-2103029469)
ACK c99063e902c810dde1742696b3140610120d391c

(Ran `git range-diff 3bc131a~2..3bc131a c99063e~2..c99063e`).

💬 cbergqvist commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1630102455)
Why not use the ability to put the descriptions inside the `static_assert()`s here and elsewhere?
```suggestion
static_assert(std::is_integral_v<I> && std::is_unsigned_v<I> && std::numeric_limits<I>::radix == 2,
"Only binary, unsigned, integer, types allowed.");
```

💬 sipa commented on pull request "util: add BitSet":
(https://github.com/bitcoin/bitcoin/pull/30160#discussion_r1630120206)
I think the condition is sufficiently self-descriptive that this isn't really needed (the compiler error emitted will report the line causing it anyway). And I still like to have a comment on all top-level statements/definitions inside a class, even if it's trivial, just for organization purposes.

💬 murchandamus commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-2153274374)
I stumbled upon a fuzz seed that timed out in CI for `package_rbf`. Since the seed appears to be too big to paste, I provide the output of `$ base64 95a3b1b42d43ac7190f24ff116b2cd6515e7a566 > /tmp/base64-95a3b1b4-oom.txt`:
[base64-95a3b1b4-oom.txt](https://github.com/user-attachments/files/15685350/base64-95a3b1b4-oom.txt)

💬 marcofleon commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1630144097)
SAM 3.1 doesn't use ports so there's a check in `Connect` that `to.GetPort()` equals 0. For the purposes of this fuzz test it doesn't seem to really matter whether `to` is specified. But I agree it's not best practice to have it be uninitialized. I think changing this to `if (session.Connect(CService{}, conn, proxy_error))` and getting rid of the declared variable is probably better.

💬 pinheadmz commented on pull request "include verbose "debug-message" field in testmempoolaccept response":
(https://github.com/bitcoin/bitcoin/pull/28121#issuecomment-2153316720)
Thanks for reviewing @jonatack I like the idea of "reject-details" and addressed your other comments as well

💬 marcofleon commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1630167230)
Good catch, thanks. Fixed.

💬 cbergqvist commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1630165558)
For me this is more straight forward:
```suggestion
if (m_offset + pos >= m_capacity) {
return (m_offset + pos) - m_capacity;
} else {
return m_offset + pos;
}
```
"We take where we begin now, `m_offset`, add `pos` to it and see what that means."
Subtractions hurt readability in this case.

👍 cbergqvist approved a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2103113644)
ACK ee253ca7dea9bed01d4c1800760477ef06310df8

💬 cbergqvist commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1630195288)
Since you removed `<T>` after `construct_at`:
```suggestion
std::destroy_at(m_buffer + m_offset);
```

💬 cbergqvist commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1630211316)
Could do with an `Assume(pos < m_capacity);` as other cases are not handled.

💬 kevkevinpal commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630232009)
thanks! added in [21bc4ff](https://github.com/bitcoin/bitcoin/pull/30195/commits/21bc4ff147cdd0a2fd37097feffbab8edfc93320)

💬 kevkevinpal commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630232408)
thanks! added in [21bc4ff](https://github.com/bitcoin/bitcoin/pull/30195/commits/21bc4ff147cdd0a2fd37097feffbab8edfc93320)

💬 kevkevinpal commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630233306)
thanks! added in [21bc4ff](https://github.com/bitcoin/bitcoin/pull/30195/commits/21bc4ff147cdd0a2fd37097feffbab8edfc93320)

💬 kevkevinpal commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630234249)
thanks! added in [21bc4ff](https://github.com/bitcoin/bitcoin/pull/30195/commits/21bc4ff147cdd0a2fd37097feffbab8edfc93320)

💬 kevkevinpal commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630237777)
thanks! added in [21bc4ff](https://github.com/bitcoin/bitcoin/pull/30195/commits/21bc4ff147cdd0a2fd37097feffbab8edfc93320)

One thing I did notice is that if I did not add the full path in `.rename` the second time I call `.rename` it cannot find the correct file and when I check the blocks dir manually it seemed like it never changed the file name. But oddly the rpc error is still seen

I might do some more investigation why I'm seeing that, but let me know if this looks okay now

💬 kevkevinpal commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#issuecomment-2153398610)
Thank you @tdb3 and @AngusP for the reviews! I added the changes minus one issue I had with renaming the file

👍 ismaelsadeeq approved a pull request: "policy: bump TX_MAX_STANDARD_VERSION to 3"
(https://github.com/bitcoin/bitcoin/pull/29496#pullrequestreview-2103208798)
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c 🛰️

💬 sipa commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1630274211)
Done partially. `if (m_offset + pos >= m_capacity) {` might be wrong in the hypothetical case that `m_offset + pos` overflows (which would require a buffer larger than half of the address space...), so I've left a comment instead.

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