💬 RCasatta commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1529027307)
> Writing to stdout would mean that the data would have to be carried over the rpc connection, right?

Yes, would that be a problem?

💬 sipa commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1529027970)
There is no way we can currently send gigabytes of data as an RPC response; both the server and client likely buffer the result and would OOM.

💬 aureleoules commented on pull request "rpc: Optimize serialization disk space of dumptxoutset":
(https://github.com/bitcoin/bitcoin/pull/26045#issuecomment-1529041350)
Thanks for the patch @TheCharlatan, I applied it.

💬 kcalvinalvin commented on pull request "index: Compare deserialized block hash with the block hash from the blockindex":
(https://github.com/bitcoin/bitcoin/pull/26390#issuecomment-1529058465)
Rebased on master

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