📝 cefikhan opened a pull request: "ScriptToUniv can get address of PUBKEY"
(https://github.com/bitcoin/bitcoin/pull/27546)
rpc: ScriptToUniv Can get address of PUBKEY

💬 sipa commented on pull request "ScriptToUniv can get address of PUBKEY":
(https://github.com/bitcoin/bitcoin/pull/27546#issuecomment-1528961205)
Concept NACK

Sorry, no. P2PK outputs do not *have* an address. It's an outdated and confusing practice to refer to them as their corresponding P2PKH address.

💬 MarcoFalke commented on pull request "ScriptToUniv can get address of PUBKEY":
(https://github.com/bitcoin/bitcoin/pull/27546#issuecomment-1528974868)
Closing for now, because this seems to be controversial and all tests fail, so this can't be merged in any case. Let us know if you have any questions, or you can refer to stack exchange:

General bitcoin questions and/or support requests are best directed to the [Bitcoin StackExchange](https://bitcoin.stackexchange.com) or the `#bitcoin` IRC channel on Libera Chat.

✅ MarcoFalke closed a pull request: "ScriptToUniv can get address of PUBKEY"
(https://github.com/bitcoin/bitcoin/pull/27546)

💬 MarcoFalke commented on pull request "test: add coverage for `-bantime`":
(https://github.com/bitcoin/bitcoin/pull/26604#discussion_r1181194479)
Ah sorry. You are checking the ban_duration, not time_remaining. So this is fine. For reference, the output with the CPU put to sleep for a second looked like:

```json
{
"address": "127.0.0.1/32",
"ban_created": 1682845373,
"banned_until": 1682846607,
"ban_duration": 1234,
"time_remaining": 1233
}

💬 MarcoFalke commented on pull request "test: add coverage for `-bantime`":
(https://github.com/bitcoin/bitcoin/pull/26604#issuecomment-1528976262)
lgtm ACK 9c18992bbaf649f8c5461d5e4dc39eb1a07ffc77

🤔 TheCharlatan reviewed a pull request: "rpc: simplify scan blocks"
(https://github.com/bitcoin/bitcoin/pull/26780#pullrequestreview-1407084457)
Concept ACK

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

💬 TheCharlatan commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#discussion_r1181200641)
Isn't this set already in line 2392?

💬 TheCharlatan commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#discussion_r1181200751)
To me the semantics of `stop_block` seem very similar. How about checking it as well?

💬 TheCharlatan commented on pull request "rpc: simplify scan blocks":
(https://github.com/bitcoin/bitcoin/pull/26780#discussion_r1181208231)
For reference,`end_range->nHeight` would be a bit easier to read, but is not guaranteed to have a value.

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