💬 TheCharlatan commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1529067823)
I ran some more numbers, dumping a mainnet snapshot on height 787535:

| Filename | Size (bytes) |
|-----------------|--------------|
| this_branch.dat | 4'697'836'541 |
| this_branch.tar.xz | 3'899'452'268 |
| master.dat | 5'718'308'597 |
| master.tar.xz | 3'925'608'176 |

So even in the compressed form the encoding saves some megabytes.

🤔 furszy reviewed a pull request: "index: Compare deserialized block hash with the block hash from the blockindex"
(https://github.com/bitcoin/bitcoin/pull/26390#pullrequestreview-1407151959)
Code reviewed, left few comments. Nothing big.

💬 furszy commented on pull request "index: Compare deserialized block hash with the block hash from the blockindex":
(https://github.com/bitcoin/bitcoin/pull/26390#discussion_r1181267848)
nit:
Better if we call it `expected_block_hash`, and also rename the one called `hash` to `expected_filter_hash`.

💬 furszy commented on pull request "index: Compare deserialized block hash with the block hash from the blockindex":
(https://github.com/bitcoin/bitcoin/pull/26390#discussion_r1181267954)
This error would be easier to debug if it logs an specific error like "Block hash mismatch in filter decode".

Plus, could leave this as it was before if the new arg is called `expected_block_hash` (no need to do the `read_block_hash` rename).

💬 furszy commented on pull request "index: Compare deserialized block hash with the block hash from the blockindex":
(https://github.com/bitcoin/bitcoin/pull/26390#discussion_r1181268939)
Could use structured bindings:
```c++
for (const auto& [block_hash, filter] : entries) {
if (!ReadFilterFromDisk(filter.pos, filter.hash, block_hash, *filter_pos_it)) {
```

(Same below)

💬 furszy commented on pull request "index: Compare deserialized block hash with the block hash from the blockindex":
(https://github.com/bitcoin/bitcoin/pull/26390#discussion_r1181270062)
Wouldn't hurt to add a comment for this. By reading only the function's signature, it's not clear what the uin256 is.

💬 furszy commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#issuecomment-1529099811)
Thanks for the review TheCharlatan.

> Does this require a release note for the s/filtertype/filter_type/ change?

Yeah.. I made this in December, just after #23549 got merged. Now that v25 is released, we have to add release notes for it.
Will just drop that commit.

💬 furszy commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#discussion_r1181271406)
yeah, thanks.

💬 furszy commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#discussion_r1181271614)
sounds good

💬 furszy commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#discussion_r1181272996)
yeah, the user could have aborted the process at the very beginning.

maybe could change it to:
```
end_range ? end_range->nHeight : start_index->nHeight;
```
but I'm not so sure that this makes code easier to read.

👍 TheCharlatan approved a pull request: "[23.x] Additional backports for 23.x"
(https://github.com/bitcoin/bitcoin/pull/27475#pullrequestreview-1407167093)
ACK f0919339bfd983975fe3b85f51423302a1d8a5a0

💬 TheCharlatan commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1181281332)
I'm guessing the commit message should say "rest:..."?

🤔 TheCharlatan reviewed a pull request: "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block"
(https://github.com/bitcoin/bitcoin/pull/26415#pullrequestreview-1407169071)
Concept ACK

Code looks good to me, but still want to run some tests.

💬 TheCharlatan commented on pull request "build: Check usages of #if defined(...)":
(https://github.com/bitcoin/bitcoin/pull/25302#discussion_r1181287798)
I think this is only defined conditionally.

📝 aureleoules opened a pull request: "ci: Add test coverage job"
(https://github.com/bitcoin/bitcoin/pull/27547)
This PR adds a new Cirrus job that generates test coverage reports for all pull requests and master using [Codecov](https://about.codecov.io/). It is free for open-source projects.
We can now quickly determine whether a feature that a pull request adds is properly tested.

I've ran a few tests on my own fork, you can check it out here: https://app.codecov.io/gh/aureleoules/bitcoin and a test pull request: https://app.codecov.io/gh/aureleoules/bitcoin/pull/5.

Codecov has a nice feature that
...

💬 achow101 commented on issue "Consider unconfirmed unspent outputs from external wallet as safe for spending for "sendtoaddress" and "fundrawtransaction" RPC":
(https://github.com/bitcoin/bitcoin/issues/17936#issuecomment-1529141419)
Such inputs can be manually selected when using `fundrawtransaction` or `walletcreatefundedpsbt`. However allowing coin selection to select such inputs introduces a potential footgun and we generally are not interested in doing that.

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always w
...

✅ achow101 closed an issue: "Consider unconfirmed unspent outputs from external wallet as safe for spending for "sendtoaddress" and "fundrawtransaction" RPC"
(https://github.com/bitcoin/bitcoin/issues/17936)

✅ achow101 closed an issue: "Prevent possible footgun caused by decodescript converting P2PKH with uncompressed pubkey into SegWit addresses"
(https://github.com/bitcoin/bitcoin/issues/19383)

💬 achow101 commented on issue "Prevent possible footgun caused by decodescript converting P2PKH with uncompressed pubkey into SegWit addresses":
(https://github.com/bitcoin/bitcoin/issues/19383#issuecomment-1529141656)
We generally avoid showing things to users that can result in funds being "stuck", even if technically still valid.

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

💬 ariard commented on issue "Package Relay Project Tracking":
(https://github.com/bitcoin/bitcoin/issues/27463#issuecomment-1529256915)
If the state-of-art implementation of BIP331 can be linked here or in the BIP (Like there is https://github.com/glozow/bitcoin/pull/8 and https://github.com/glozow/bitcoin/pull/9) ?

From a reviewer perspective, ideally it’s better to have the proposed specification changes, the proposed Core code changes and the backend code of broadcaster client (e.g Lightning) to grasp a correct mental model of all the interactions.