💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659968285)
same, it seems to me that it's not the head that's tested, but its next pointer
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659968285)
same, it seems to me that it's not the head that's tested, but its next pointer
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659967194)
instead of exposing `GetFlags`, we could check that it's not fresh and not dirty
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1659967194)
instead of exposing `GetFlags`, we could check that it's not fresh and not dirty
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660017722)
It isn't because entry is constructed the line above, and we know from default constructor `flags` will be initialized to `0`. So `0 | flags` is same as `flags`.
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660017722)
It isn't because entry is constructed the line above, and we know from default constructor `flags` will be initialized to `0`. So `0 | flags` is same as `flags`.
💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198357675)
> > thanks @tdb3 and @alfonsoromanz I added a log in [82c454c](https://github.com/bitcoin/bitcoin/pull/30340/commits/82c454c65658439481b50fe71ed097ecb8d70737)
> > Also @tdb3 I tried doing
> > ```
> > b1hash = index_node.getblockhash(1)
> > index_node.invalidateblock(b1hash)
> >
> > self.log.info("Test obtaining info for a non-existent block hash")
> > assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)
> >
...
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198357675)
> > thanks @tdb3 and @alfonsoromanz I added a log in [82c454c](https://github.com/bitcoin/bitcoin/pull/30340/commits/82c454c65658439481b50fe71ed097ecb8d70737)
> > Also @tdb3 I tried doing
> > ```
> > b1hash = index_node.getblockhash(1)
> > index_node.invalidateblock(b1hash)
> >
> > self.log.info("Test obtaining info for a non-existent block hash")
> > assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)
> >
...
📝 Balcoin-BLC opened a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/30363)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30363)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
📝 achow101 locked a pull request: "Update README.md"
(https://github.com/bitcoin/bitcoin/pull/30363)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/30363)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 tdb3 commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198387274)
> > Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn't it be better to use something guaranteed not to collide? Something above the max target for regtest?
>
> sounds good that makes sense I just pushed [837af6e](https://github.com/bitcoin/bitcoin/pull/30340/commits/837af6e
...
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198387274)
> > Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn't it be better to use something guaranteed not to collide? Something above the max target for regtest?
>
> sounds good that makes sense I just pushed [837af6e](https://github.com/bitcoin/bitcoin/pull/30340/commits/837af6e
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660083440)
Indeed, same for line 130
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660083440)
Indeed, same for line 130
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660086532)
Not sure if it helps, just an observation: `m_prev` only uses `.second`, so it might as well be a `CCoinsCacheEntry*`
```patch
Index: src/coins.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/coins.h b/src/coins.h
--- a/src/coins.h (revision c36363f6b24c7ab2afe198d9855f507ddf096e1f)
+++ b/src/coins.h (date 1719730674870)
@@ -123,7 +123,7 @@
* parent cac
...
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660086532)
Not sure if it helps, just an observation: `m_prev` only uses `.second`, so it might as well be a `CCoinsCacheEntry*`
```patch
Index: src/coins.h
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/src/coins.h b/src/coins.h
--- a/src/coins.h (revision c36363f6b24c7ab2afe198d9855f507ddf096e1f)
+++ b/src/coins.h (date 1719730674870)
@@ -123,7 +123,7 @@
* parent cac
...
💬 Fi3 commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2198494395)
@itornaza I agree with you about the implication of integrating noise into the TP. Just want to point out 2 minors issues [in your sketch](https://github.com/user-attachments/files/16033413/sv2-draft-sketch.pdf):
1. Pool and JDS do not need to communicate (and if they do they do not use the job declaration protocol)
2. JDS and TP do not communicate using the template distribution protocol. JDS <-> TP communication is not part of Sv2. For example the JDS on SRI just use bitcoind RPC API.
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2198494395)
@itornaza I agree with you about the implication of integrating noise into the TP. Just want to point out 2 minors issues [in your sketch](https://github.com/user-attachments/files/16033413/sv2-draft-sketch.pdf):
1. Pool and JDS do not need to communicate (and if they do they do not use the job declaration protocol)
2. JDS and TP do not communicate using the template distribution protocol. JDS <-> TP communication is not part of Sv2. For example the JDS on SRI just use bitcoind RPC API.
💬 itornaza commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2198513621)
@Fi3 thank you for clarifying it really helped a lot! Would you also think this is a good point to start reviewing the Noise Protocol implementation on this PR compared to it's [specification](https://noiseprotocol.org/noise.html)?
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2198513621)
@Fi3 thank you for clarifying it really helped a lot! Would you also think this is a good point to start reviewing the Noise Protocol implementation on this PR compared to it's [specification](https://noiseprotocol.org/noise.html)?
💬 fjahr commented on pull request "test: fix inconsistency in fundrawtransaction weight limits test":
(https://github.com/bitcoin/bitcoin/pull/30353#issuecomment-2198529040)
Code review ACK 4f571fb73526b55a83af3c04fbc662ccdd1307ae
This looks like the right fix for the observed test failure to me.
(https://github.com/bitcoin/bitcoin/pull/30353#issuecomment-2198529040)
Code review ACK 4f571fb73526b55a83af3c04fbc662ccdd1307ae
This looks like the right fix for the observed test failure to me.
💬 txxlt commented on issue "importaddress failed, Only legacy wallets are supported by this command.":
(https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2198568437)
损失dumpprivkey是令人难以接受的，自以为是的蠢蛋们正在毁灭BTC
(https://github.com/bitcoin/bitcoin/issues/29772#issuecomment-2198568437)
损失dumpprivkey是令人难以接受的，自以为是的蠢蛋们正在毁灭BTC
🤔 paplorinc reviewed a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2150053762)
I still think we can get rid of `m_prev` by always using an immutable `SENTINEL` as the last element, inserting new elements at the head, and handling deletions by copying the next node's content and deleting that instead (unless it's the `SENTINEL`).
What do you think?
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2150053762)
I still think we can get rid of `m_prev` by always using an immutable `SENTINEL` as the last element, inserting new elements at the head, and handling deletions by copying the next node's content and deleting that instead (unless it's the `SENTINEL`).
What do you think?
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660183610)
nit: it seems to me we're really pushing it with the loop iteration + deletion here, could we do the deletion at the end of the loop instead to have a cleaner separation or the responsibilities? It may make the `Next` method cleaner as well (I know it was similar before as well).
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660183610)
nit: it seems to me we're really pushing it with the loop iteration + deletion here, could we do the deletion at the end of the loop instead to have a cleaner separation or the responsibilities? It may make the `Next` method cleaner as well (I know it was similar before as well).
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660189469)
This kept me up at night, we should be able to get rid of `m_prev` by doing insertions and deletions slightly differently:
* we'd have an immutable `SENTINEL` value always serving as the last element (instesd of `nullptr`);
* insertion would always be in front of the previous element, which would serve as the new head (iterating from which will always get us to the `SENTINEL`);
* deletion would copy the content (`node`, `flag` and `next`) of the next node instead (unless it's a `SENTINEL`), a
...
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660189469)
This kept me up at night, we should be able to get rid of `m_prev` by doing insertions and deletions slightly differently:
* we'd have an immutable `SENTINEL` value always serving as the last element (instesd of `nullptr`);
* insertion would always be in front of the previous element, which would serve as the new head (iterating from which will always get us to the `SENTINEL`);
* deletion would copy the content (`node`, `flag` and `next`) of the next node instead (unless it's a `SENTINEL`), a
...
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660184345)
after iteration we may still have elements left in `node`, right? Can we assert that we don't?
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660184345)
after iteration we may still have elements left in `node`, right? Can we assert that we don't?
💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660182664)
we're inside `CCoinsCacheEntry`, might as well omit the prefix:
```suggestion
inline bool IsDirty() const { return m_flags & DIRTY; }
inline bool IsFresh() const { return m_flags & FRESH; }
```
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1660182664)
we're inside `CCoinsCacheEntry`, might as well omit the prefix:
```suggestion
inline bool IsDirty() const { return m_flags & DIRTY; }
inline bool IsFresh() const { return m_flags & FRESH; }
```
💬 kevkevinpal commented on pull request "test: Added coverage to Block not found error using gettxoutsetinfo":
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198587045)
> I could be off, but the regtest powlimit may be:
>
> https://github.com/bitcoin/bitcoin/blob/2f6dca4d1c01ee47275a4292f128d714736837a1/src/kernel/chainparams.cpp#L423
>
> which means we would want to use a value higher than that for the test (i.e. a hash that would have no chance of colliding with a generated block). Someone should correct if I'm mistaken.
>
> As a sanity check, I generated 10,000 blocks on regtest and none had a block hash greater than 0x7ff...
Thats a good idea! I
...
(https://github.com/bitcoin/bitcoin/pull/30340#issuecomment-2198587045)
> I could be off, but the regtest powlimit may be:
>
> https://github.com/bitcoin/bitcoin/blob/2f6dca4d1c01ee47275a4292f128d714736837a1/src/kernel/chainparams.cpp#L423
>
> which means we would want to use a value higher than that for the test (i.e. a hash that would have no chance of colliding with a generated block). Someone should correct if I'm mistaken.
>
> As a sanity check, I generated 10,000 blocks on regtest and none had a block hash greater than 0x7ff...
Thats a good idea! I
...
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198587395)
> we should be able to get rid of m_prev by doing insertions and deletions slightly differently
But we can't move the coin from the next entry to us. The `coinsCache` map will still point to the next entry for that outpoint.
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2198587395)
> we should be able to get rid of m_prev by doing insertions and deletions slightly differently
But we can't move the coin from the next entry to us. The `coinsCache` map will still point to the next entry for that outpoint.