💬 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.

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

👍 ismaelsadeeq approved a pull request: "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN"
(https://github.com/bitcoin/bitcoin/pull/30212#pullrequestreview-2103261626)
Code Review ACK aa422bcb898b7e3aa8dca912d92016bd22cf243d

💬 ismaelsadeeq commented on pull request "rename TransactionErrors: MISSING_INPUTS and ALREADY_IN_CHAIN":
(https://github.com/bitcoin/bitcoin/pull/30212#discussion_r1630291371)
Nice, I think we also rename the one in `TxValidationResult` enum.

g/`TX_MISSING_INPUTS`/`TX_MISSING_INPUT_OR_SPENT`/g
and update comments appropriately.

💬 ismaelsadeeq commented on pull request "validation: Improve, document and test logic for chains building on invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/30207#issuecomment-2153455082)
Concept ACK

💬 cbergqvist commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1630315049)
Excellent catch! I got a bit afraid that `(m_offset + pos) - m_capacity` might also run into issues when overflowing but appears to work out (and unsigned integer overflow is not undefined behavior).

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

💬 AngusP commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630316132)
Hrm, I can't check just now but I'd guess either `.rename` returns a new path instance that you should use for further changes instead, or you can try call `.resolve()` on it after rename? (Resolves to an absolute path, maybe needed if the original changed?)