💬 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?)

💬 tdb3 commented on pull request "test: Added test coverage to listsinceblock rpc":
(https://github.com/bitcoin/bitcoin/pull/30195#discussion_r1630361847)
https://docs.python.org/3/library/pathlib.html#pathlib.Path.rename

> The target path may be absolute or relative. Relative paths are interpreted relative to the current working directory, not the directory of the Path object.

Maybe this?

👍 tdb3 approved a pull request: "test: Added test coverage to listsinceblock rpc"
(https://github.com/bitcoin/bitcoin/pull/30195#pullrequestreview-2103331532)
Thanks for the updates.
ACK for 21bc4ff147cdd0a2fd37097feffbab8edfc93320

👍 theStack approved a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2103309868)
ACK eeea0818c1a20adc5225b98b185953d386c033e0

Nice catch. Took me a while to see where b47bd959207e82555f07e028cc2246943d32d4c3 introduced the bug (the retry logic is confusing indeed!), but both the detailed commit messages and [yesterday's PR review club notes/log](https://bitcoincore.reviews/30132) were very helpful to grok it.

💬 theStack commented on pull request "indexes: Don't wipe indexes again when continuing a prior reindex":
(https://github.com/bitcoin/bitcoin/pull/30132#discussion_r1630338668)
nit:
```suggestion
db_path = node.chain_path / 'indexes' / 'blockfilter' / 'basic' / 'db'
```

💬 achow101 commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#issuecomment-2153538986)
ACK fa52e13ee81fcc7543890dbd6986fcb55168583f

🚀 achow101 merged a pull request: "test: Remove struct.pack from almost all places"
(https://github.com/bitcoin/bitcoin/pull/29401)

💬 SergioDemianLerner commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-2153594153)
BitVM1 and BitVMX (and probably all of their variations such as BitVM2, SNARKnado and BitSNARK) relies on long scripts. These scripts many times contain lookup tables to implement efficiently bitwise operations since they were disabled by satoshi. The tables are generally built as a list of OP_PUSH instructions where the data pushed is always shorter than 32 bits, and code could push several hundreds items (<1000).

Also BitVM-like scripts push many Lamport public keys. These are 20 or 32 byt
...

🤔 tdb3 reviewed a pull request: "json-rpc 2.0 followups: docs, tests, cli"
(https://github.com/bitcoin/bitcoin/pull/30238#pullrequestreview-2103363648)
Approach ACK.
Solid follow up from the JSON RPC 2.0 work.
Left a few comments/notes.

Also ran all unit/functional tests (all passed). During functional test execution, did a spot check in Wireshark (e.g. for a `getblockchaininfo` call by a test) to ensure `"jsonrpc": "2.0"` was included (it was).

💬 tdb3 commented on pull request "json-rpc 2.0 followups: docs, tests, cli":
(https://github.com/bitcoin/bitcoin/pull/30238#discussion_r1630466396)
It's great to keep the specifics in a unified location (i.e. `doc/JSON-RPC-interface.md`).
Looks like the `JSON-RPC 1.1 vs 2.0` table (https://github.com/bitcoin/bitcoin/blob/master/doc/JSON-RPC-interface.md#json-rpc-11-vs-20) covers the differences in behavior for 200/404/500, `error`/`result` both vs one being present. Didn't see 204 mentioned. How about something like:

```diff
diff --git a/doc/JSON-RPC-interface.md b/doc/JSON-RPC-interface.md
index 2a97aa351d0..a6abb7e569b 100644
---
...

💬 tdb3 commented on pull request "json-rpc 2.0 followups: docs, tests, cli":
(https://github.com/bitcoin/bitcoin/pull/30238#discussion_r1630396082)
Since this line is being changed, maybe can also remove the extraneous semicolon after `application/json`.

See https://github.com/bitcoin/bitcoin/pull/30215#pullrequestreview-2092102442

💬 tdb3 commented on pull request "json-rpc 2.0 followups: docs, tests, cli":
(https://github.com/bitcoin/bitcoin/pull/30238#discussion_r1630516372)
Verified that `bitcoin-cli -netinfo`'s requests to `getpeerinfo`, and `getnetworkinfo` specify jsonrpc 2.0 successfully (as well as the resultant responses).

💬 tdb3 commented on pull request "json-rpc 2.0 followups: docs, tests, cli":
(https://github.com/bitcoin/bitcoin/pull/30238#discussion_r1630396293)
Same here

💬 tdb3 commented on pull request "json-rpc 2.0 followups: docs, tests, cli":
(https://github.com/bitcoin/bitcoin/pull/30238#discussion_r1630514245)
Verified that `bitcoin-cli -getinfo`'s requests to `getnetworkinfo`, `getblockchaininfo`, `getwalletinfo`, `getbalances`, and `listwallets` specify jsonrpc 2.0 successfully (as well as the resultant responses).