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

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