💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2214797055)
Thanks for the reviews! I force-pushed following [@dergoegge's suggestion](https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789) of doing the version message push in `SendMessages` instead, with the advantage of not needing to change the `NetEventsInterface` (sorry for invalidating the ACK @maflcko).
The previous version is still available on the following branch: https://github.com/theStack/bitcoin/tree/202407-p2p-fix_selfdetection_racecond_oldversion
(https://github.com/bitcoin/bitcoin/pull/30394#issuecomment-2214797055)
Thanks for the reviews! I force-pushed following [@dergoegge's suggestion](https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789) of doing the version message push in `SendMessages` instead, with the advantage of not needing to change the `NetEventsInterface` (sorry for invalidating the ACK @maflcko).
The previous version is still available on the following branch: https://github.com/theStack/bitcoin/tree/202407-p2p-fix_selfdetection_racecond_oldversion
💬 theStack commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1669035582)
Done as suggested.
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1669035582)
Done as suggested.
💬 mzumsande commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2214820310)
> maybe someone can confirm that everything is deterministic and the snapshot can be loaded now (e.g. by adding an assert and setting `base_blockheight` and `m_coins_count` to 200 it should crash almost immediately).
FYI, confirmed for myself on another computer that this was just an error, and the snapshot can be loaded by the fuzzer.
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2214820310)
> maybe someone can confirm that everything is deterministic and the snapshot can be loaded now (e.g. by adding an assert and setting `base_blockheight` and `m_coins_count` to 200 it should crash almost immediately).
FYI, confirmed for myself on another computer that this was just an error, and the snapshot can be loaded by the fuzzer.
🚀 ryanofsky merged a pull request: "psbt: Check non witness utxo outpoint early"
(https://github.com/bitcoin/bitcoin/pull/29855)
(https://github.com/bitcoin/bitcoin/pull/29855)
🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2164003242)
nice, I like this version a lot more.
I haven't investigated it yet, but now that we have a circular list, does the double linking still offer a measurable advantage or could we do the same with a single link and maybe 2 iterations instead (and would it be worth it, if it's possible)?
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2164003242)
nice, I like this version a lot more.
I haven't investigated it yet, but now that we have a circular list, does the double linking still offer a measurable advantage or could we do the same with a single link and maybe 2 iterations instead (and would it be worth it, if it's possible)?
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1669052113)
it seems to me that `next` remains unchanged after clear:
```C++
auto next{node->second.Next()};
node->second.ClearFlags();
assert(next == node->second.Next());
node = next;
```
so if that's intentional, we can simplify to:
```C++
node->second.ClearFlags();
node = node->second.Next();
```
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1669052113)
it seems to me that `next` remains unchanged after clear:
```C++
auto next{node->second.Next()};
node->second.ClearFlags();
assert(next == node->second.Next());
node = next;
```
so if that's intentional, we can simplify to:
```C++
node->second.ClearFlags();
node = node->second.Next();
```
💬 mzumsande commented on issue "RPC `getblock` resulted in 500 and ReadBlockFromDisk: OpenBlockFile failed for FlatFilePos(nFile=-1, nPos=0)":
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-2214874858)
If the block is not available anymore, we already throw a specific error: "Block not available (pruned data)": [rpc](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rpc/blockchain.cpp#L586-L588) / [rest](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rest.cpp#L310-L312)
So I think it would only be consistent to add a check for `BLOCK_HAVE_DATA`, not call `ReadRawBlockFromDisk` and return a similar error (e.g. "Block n
...
(https://github.com/bitcoin/bitcoin/issues/20978#issuecomment-2214874858)
If the block is not available anymore, we already throw a specific error: "Block not available (pruned data)": [rpc](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rpc/blockchain.cpp#L586-L588) / [rest](https://github.com/bitcoin/bitcoin/blob/1f9d30744d32d24ad3128721cf5bd65a3f1543e8/src/rest.cpp#L310-L312)
So I think it would only be consistent to add a check for `BLOCK_HAVE_DATA`, not call `ReadRawBlockFromDisk` and return a similar error (e.g. "Block n
...
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214881383)
@paplorinc With only a single link, how would you delete arbitrary map entries? Barring large hacks like putting the data in the next entry rather than the entry which the key points to, I don't see how you could delete entries in less than $\mathcal{O}(n)$ time.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214881383)
@paplorinc With only a single link, how would you delete arbitrary map entries? Barring large hacks like putting the data in the next entry rather than the entry which the key points to, I don't see how you could delete entries in less than $\mathcal{O}(n)$ time.
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1669095380)
this was unintentional. Disallowed it and added coverage.
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1669095380)
this was unintentional. Disallowed it and added coverage.
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214900126)
@theStack took test coverage suggestion, and yes "fees" is intentional since it will be in any net's address
@luke-jr I think @petertodd has stated what I would say. It's a pattern people can already do, and it's trivial and cheap to clean up.
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214900126)
@theStack took test coverage suggestion, and yes "fees" is intentional since it will be in any net's address
@luke-jr I think @petertodd has stated what I would say. It's a pattern people can already do, and it's trivial and cheap to clean up.
💬 luke-jr commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214922582)
>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
And at a reduced cost. Unless this proposal increases vsize to make it identical?
> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with "spam".
What is the 0x4e73 for?
These outputs bloat the UTXO set, without sufficient value to incentivise cleanup
...
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214922582)
>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
And at a reduced cost. Unless this proposal increases vsize to make it identical?
> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It has nothing to do with "spam".
What is the 0x4e73 for?
These outputs bloat the UTXO set, without sufficient value to incentivise cleanup
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214968788)
> With only a single link, how would you delete arbitrary map entries?
My naive thinking was that maybe we don't actually need random-element deletion. Based on how `ClearFlags` and `erase` are currently called from loops that already have the previous node, we could often call `ClearFlags(*prev)`.
The destructor is trickier. Maybe we can handle this in the `CCoinsViewCache` destructor instead, batch-destructing entries there, which would involve a single extra $\mathcal{O}(n)$ traversal.
I
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214968788)
> With only a single link, how would you delete arbitrary map entries?
My naive thinking was that maybe we don't actually need random-element deletion. Based on how `ClearFlags` and `erase` are currently called from loops that already have the previous node, we could often call `ClearFlags(*prev)`.
The destructor is trickier. Maybe we can handle this in the `CCoinsViewCache` destructor instead, batch-destructing entries there, which would involve a single extra $\mathcal{O}(n)$ traversal.
I
...
💬 petertodd commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214969086)
On July 8, 2024 8:42:50 PM GMT+02:00, Luke Dashjr ***@***.***> wrote:
>>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
>
>And at a reduced cost. Unless this proposal increases vsize to make it identical?
The cost reduction is a natural one, in line with the byte reduction.
>> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It
...
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2214969086)
On July 8, 2024 8:42:50 PM GMT+02:00, Luke Dashjr ***@***.***> wrote:
>>This proposal is merely a way of doing something that is already done - signed anchor outputs - in a more efficient way with less overall bytes consumed.
>
>And at a reduced cost. Unless this proposal increases vsize to make it identical?
The cost reduction is a natural one, in line with the byte reduction.
>> The creating and spending of keyless anchor outputs is completely fixed, with no ability to encode any data. It
...
💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214976515)
@paplorinc When UTXOs are spent while FRESH, they get deleted from the cache; this is quite possibly the most significant performance optimization Bitcoin Core has during IBD, as it avoids the need for those UTXOs to ever end up on disk. It's done based on `COutPoint` lookup, not during iteration.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214976515)
@paplorinc When UTXOs are spent while FRESH, they get deleted from the cache; this is quite possibly the most significant performance optimization Bitcoin Core has during IBD, as it avoids the need for those UTXOs to ever end up on disk. It's done based on `COutPoint` lookup, not during iteration.
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214993679)
Makes sense, thanks for explaining @sipa!
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2214993679)
Makes sense, thanks for explaining @sipa!
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669189198)
This code is gone.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1669189198)
This code is gone.
💬 instagibbs commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2215089061)
> What is the 0x4e73 for?
To be segwit, it needs to be a witness version + witness program(or p2sh nested segwit script). A witness program is 2 to 40 bytes long push. For this use-case we simply use size two. The specific bytes chosen are simply "cutesy", as they cannot be 0 so something else has to be chosen and have an associated address. As far as I know this is the only proposed size 2 witness program use case so far.
> These outputs bloat the UTXO set, without sufficient value to i
...
(https://github.com/bitcoin/bitcoin/pull/30352#issuecomment-2215089061)
> What is the 0x4e73 for?
To be segwit, it needs to be a witness version + witness program(or p2sh nested segwit script). A witness program is 2 to 40 bytes long push. For this use-case we simply use size two. The specific bytes chosen are simply "cutesy", as they cannot be 0 so something else has to be chosen and have an associated address. As far as I know this is the only proposed size 2 witness program use case so far.
> These outputs bloat the UTXO set, without sufficient value to i
...
🤔 mzumsande reviewed a pull request: "Assumeutxo: bugfix on loadtxoutset with a divergent chain + test"
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2164290570)
Code Review ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c
(https://github.com/bitcoin/bitcoin/pull/29996#pullrequestreview-2164290570)
Code Review ACK 5b7f70ba2661a194a3c476b81e03785feddb4e1c
🤔 fjahr reviewed a pull request: "assumeutxo: Don't load a snapshot if it's not in the best header chain"
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2164298309)
re-ACK ab478c5fa16427b496e8a93e4780770d4c270214
Confirmed only changes since last review were the rebase and addressing comments (per `git range-diff master 29ab88dfe1c43af3620480c99cba844aa414023c ab478c5fa16427b496e8a93e4780770d4c270214`).
(https://github.com/bitcoin/bitcoin/pull/30320#pullrequestreview-2164298309)
re-ACK ab478c5fa16427b496e8a93e4780770d4c270214
Confirmed only changes since last review were the rebase and addressing comments (per `git range-diff master 29ab88dfe1c43af3620480c99cba844aa414023c ab478c5fa16427b496e8a93e4780770d4c270214`).
💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1669238451)
nit: seems the "snapshot" added before is back out, still not a block for me though
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1669238451)
nit: seems the "snapshot" added before is back out, still not a block for me though