💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469402366)
Added

📝 stickies-v opened a pull request: "test: fix wallet_import_rescan unrounded minimum amount"
(https://github.com/bitcoin/bitcoin/pull/29343)
Addresses https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1468842089.

Fixes a `JSONRPCException: Invalid amount (-3)` exception by ensuring the amount sent to `sendtoaddress` is rounded to 8 decimals.

See https://cirrus-ci.com/task/5562947183837184?logs=ci#L2559

Note: since `round` can also round down, `min_amount` is not _exactly_ guaranteed, but this is not a problem for the current usage. I've added a docstring to highlight this.

💬 stickies-v commented on pull request "test: ensure output is large enough to pay for its fees":
(https://github.com/bitcoin/bitcoin/pull/29283#discussion_r1469408527)
https://github.com/bitcoin/bitcoin/pull/29343/

👍 vasild approved a pull request: "CKey: add Serialize and Unserialize"
(https://github.com/bitcoin/bitcoin/pull/29295#pullrequestreview-1848385247)
ACK 683f988f90bd625544529b2366984bb677dd6e31

💬 vasild commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469415640)
No need for the extra nesting `{`.

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914474598)
I dropped support for uncompressed keys and added a comment to explain the difference with OpenSSL encoding. Also added test for invalid keys.

💬 maflcko commented on pull request "test: Treat msg_version.relay as unsigned, Remove `struct` packing in messages.py":
(https://github.com/bitcoin/bitcoin/pull/29067#discussion_r1469441903)
> I suppose it doesn't really matter there, but would still keep that consistent with the actual type.

Thanks, done.

Also, rebased to fix the CI failure.

💬 dergoegge commented on pull request "redeclare nChainTx to use uint64_t":
(https://github.com/bitcoin/bitcoin/pull/29331#issuecomment-1914487359)
Would you mind adding a test? It should be possible to fake a large `nChainTx` in a test using assume utxo

👍 dergoegge approved a pull request: "fuzz: Print coverage summary after run_once"
(https://github.com/bitcoin/bitcoin/pull/29329#pullrequestreview-1848441332)
ACK fa4a08d424aa93f55b05fc9d83c8465599e5cece

Good idea to only collect and print the stats when using libfuzzer.

💬 maflcko commented on pull request "fuzz: Print coverage summary after run_once":
(https://github.com/bitcoin/bitcoin/pull/29329#issuecomment-1914493611)
(Added missing `if using_libfuzzer` check to fix macOS CI.)

💬 maflcko commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469449395)
template implies `inline`, so it can be removed. Also, could run clang-format on new code?

💬 maflcko commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469450644)
Any reason to cast a byte span to a u-char span? I think you can replace all `Make*Span()` by just `Span{}`.

💬 maflcko commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-1914506129)
I don't think any care close-check needs to be done when reading from a file.

So what about removing the `write` method from `AutoFile`, and introduce a new derived class to add it back. This class could `Assume` that the file was flushed/closed before the destructor is called?

✅ fanquake closed an issue: "Clang 14 emits `-Wunreachable-code` warnings"
(https://github.com/bitcoin/bitcoin/issues/29334)

💬 fanquake commented on issue "Clang 14 emits `-Wunreachable-code` warnings":
(https://github.com/bitcoin/bitcoin/issues/29334#issuecomment-1914514298)
Closing as upgrade compiler / pass a flag to disable the false positive / don't pass error flags to turn known false positives into compile failures.

👍 TheCharlatan approved a pull request: "refactor: Fix prevector iterator concept issues"
(https://github.com/bitcoin/bitcoin/pull/29275#pullrequestreview-1848469672)
ACK fad74bbbd0ab4425573f182ebde1b31a99e80547

Could the `bitdeque` iterator also get `std::contiguous_iterator_tag`?

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469469780)
Done. Would be nice to do this in a pre-commit hook or something, because it's unlikely I'm going to remember doing this every time.

💬 Sjors commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#discussion_r1469469850)
Done in both places.

💬 josibake commented on pull request "CKey: add Serialize and Unserialize":
(https://github.com/bitcoin/bitcoin/pull/29295#issuecomment-1914521350)
> Why would you not use a `CKey` to hold a private key?

My point was not "CKey shouldn't be used for private keys." My point is `CKey` (or at least my understanding of it) is meant for a *specific* kind(s) of "private key" i.e. a private key that represents a private/public key pair and is intended to be used in that context. It also seems we want to know where the key came from and ensure that it is valid, hence the only ways to load a secret key are the `DecodeSecret` method, which decodes
...

💬 t-bast commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1914523198)
> (eg how do we handle 0conf ln channels? I need to make a delving post to discuss this)

I'd be curious to see your delvingbitcoin post around that, as I believe this shouldn't be too much of an issue (but let's discuss it on a dedicated post instead of here).

> I think that pattern-matching / imbuing v3 is doable (that sounds like a concept ack for v3?). Assuming sibling eviction in v3, so far, it seems like it applies nicely to existing commitment transactions without any modifications.
...