💬 maflcko commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1905488484)
> > `$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest`
>
> Do we not consider regtest "testing"?

Regtest should have the same properties as `main` for the purposes here. It should be possible to reproduce on main as well, if you want to do the POW.

💬 maflcko commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1905546034)
lgtm ACK 19fbbb1d00f40095b0b6da5941ff069376800e7f

💬 delta1 commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1905548068)
ACK 19fbbb1

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905560370)
@maflcko `WriteBinaryFile` is used by Tor and I2P to cache the service private key.

But I might still close this PR if all that's needed is one-liner.

💬 ajtowns commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1905602751)
> btcd

libbitcoin optionally uses https://github.com/libbitcoin/libbitcoin-consensus which has a similar name, but doesn't seem to have any code in common. AFAIK, btcd reimplements all the consensus logic in go.

💬 willcl-ark commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1905642808)
Ah I see now, thanks. The repro steps threw me off there. I did now also verify that it is hit on (a custom) signet (with `-checkblocks` set).

📝 Sjors converted_to_draft a pull request: "Make (Read/Write)BinaryFile work with char vector"
(https://github.com/bitcoin/bitcoin/pull/29229)
ReadBinaryFile and WriteBinaryFile current work with `std::string`. This PR adds support for `std::vector<unsigned char>>`.

This is used in #28983 to store the static key for the Template Provider, in a manner very similar to how we store the Tor v3 and i2p key. However it made no sense to me to store a `CKey` as plain text. See commit "Persist static key for Template Provider" for how it's used.

It uses a template and leverages the fact that both `std::string` and `std::vector<unsigned ch
...

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905684497)
It's not quite a one-liner because you still need to open a close a `FILE` and deal with various errors. So instead I modified `[Write/Write]BinaryFile` to use `AutoFile` instead of `fwrite` / `fread`.

However, `AutoFile{f} >> output` only returns a fraction of the file in the test...

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905703500)
> However, `AutoFile{f} >> output` only returns a fraction of the file in the test...

(De)serialization of vectors or strings assumes the run-time length to be encoded first. Only arrays and spans assume no length, because it is assumed to be known at compile-time.

💬 fanquake commented on pull request "Avoid non-self-contained Windows header":
(https://github.com/bitcoin-core/gui/pull/789#discussion_r1463031223)
Comment should be removed? We are removing these "for xyz" comments in any case, but this one would also now be incorrect.

💬 brunoerg commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1905743813)
Concept ACK

💬 stickies-v commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1463055516)
thx, fixed

💬 brunoerg commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1905749723)
> @brunoerg?

Sounds good to me. Is the CJDNS address from `ConsumeNetAddr` always valid?

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905760878)
Ok, fixed that issue by first getting the file size and then resizing the `std::vector` / `std::string`.

That doesn't generalise nicely to known-size things like `CKey`. The `maxsize` argument also makes sense for that. So I might make a separate method for that. Which in turn makes this PR just a refactor with unused, but tested, support for `std::vector<unsigned char>`.

👋 Sjors's pull request is ready for review: "Make (Read/Write)BinaryFile work with char vector"
(https://github.com/bitcoin/bitcoin/pull/29229)

🤔 naumenkogs reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1838102663)
Some low-level feedback for now, mostly nits. I will do high-level review against the BIP shortly.

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462877634)
c950386eaaf41ec096606de81a3a60b278b156f0
nit: since `v2transport` is named exactly the same in addconnection and as an init flag, perhaps either specify where exactly to see, or have extended description in both places ("adding v2transport connections requires v2transport init flag to be set."). Current version is kinda confusing.

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462899288)
9b6425ce0fb74b40e12f92b704a41feec23cd754
nit: "at least one byte" is confusing... what if it's half-a-byte? Sounds like that would be ignored, but i assume it won't be.

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462920200)
9b6425ce0fb74b40e12f92b704a41feec23cd754

nit: might be cleaner this way
```
received_prefix_len = 0
while received_prefix_len < 16:
byte = response.read(1)
# return b"" if we need to receive more bytes
if not byte:
return received_prefix_len, b""
received_prefix_len += len(byte)
if byte != v1_prefix[received_prefix_len - 1]:
return received_prefix_len, self.generate_keypair_an
...

💬 naumenkogs commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1462949568)
9a2050275573eae31c5dc14fefedb612e951d0b8

do you mind adding a comment for what happened here?