💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662746393)
Should this function be checking that we haven't exceeded the capacity of the SetType?
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662746393)
Should this function be checking that we haven't exceeded the capacity of the SetType?
💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662824605)
Should this say "serializer" instead of "deserializer"?
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662824605)
Should this say "serializer" instead of "deserializer"?
💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662822694)
I was really confused about why we need to use `CanAddDependency` here -- as far as I can understand, it's basically about compression, so that we get shorter serializations? And conceptually, the reason compression is helpful is in the context of serializations that are produced by a fuzzer being as meaningful as possible?
I guess compression doesn't hurt here, but just to verify my understanding: would all this logic work just fine if we dropped the use of `CanAddDependency` in the seriali
...
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662822694)
I was really confused about why we need to use `CanAddDependency` here -- as far as I can understand, it's basically about compression, so that we get shorter serializations? And conceptually, the reason compression is helpful is in the context of serializations that are produced by a fuzzer being as meaningful as possible?
I guess compression doesn't hurt here, but just to verify my understanding: would all this logic work just fine if we dropped the use of `CanAddDependency` in the seriali
...
💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1664704882)
nit: if you touch up this commit, I think the commit message has a couple typos:
`This is a class that encapsulated precomputes ancestor set feerates, and`
should read "encapsulates precomputed ancestor set feerates"
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1664704882)
nit: if you touch up this commit, I think the commit message has a couple typos:
`This is a class that encapsulated precomputes ancestor set feerates, and`
should read "encapsulates precomputed ancestor set feerates"
💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662966890)
Nit: The fee/size values in this comment don't seem to match the fee/size values in the test below.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662966890)
Nit: The fee/size values in this comment don't seem to match the fee/size values in the test below.
💬 sdaftuar commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662836077)
Left another comment down below regarding `CanAddDependency()` in the serializer -- it makes sense to me to use it in the deserializer in the context of having a fuzzer produce random bytes which we try to deserialize into a graph, but I don't follow why it's necessary in the serializer.
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1662836077)
Left another comment down below regarding `CanAddDependency()` in the serializer -- it makes sense to me to use it in the deserializer in the context of having a fuzzer produce random bytes which we try to deserialize into a graph, but I don't follow why it's necessary in the serializer.
💬 TheCharlatan commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207195276)
I checked if there is anything we could do in the testing setup to make the test faster. With this patchset, the test takes around 1:15 min for 10k runs on my machine. Removing the validation interface from the test setup cuts it down to 1:05 min , if I remove the networking setup, it takes 35 sec to complete.
I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207195276)
I checked if there is anything we could do in the testing setup to make the test faster. With this patchset, the test takes around 1:15 min for 10k runs on my machine. Removing the validation interface from the test setup cuts it down to 1:05 min , if I remove the networking setup, it takes 35 sec to complete.
I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?
💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1664822525)
Yes, it is needed, for the simple reason that the compressor needs to predict what the decompressor state will be, or they will go out of sync. If they disagree about which dependencies are possible, the delta-encoded differences between them will be off, resulting in possibly widely different DepGraphs on decoding. An example where this matters is in the 4th unit test (specifically constructed for that, suggested by @instagibbs).
I am considering a simplification for the encoding format howe
...
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1664822525)
Yes, it is needed, for the simple reason that the compressor needs to predict what the decompressor state will be, or they will go out of sync. If they disagree about which dependencies are possible, the delta-encoded differences between them will be off, resulting in possibly widely different DepGraphs on decoding. An example where this matters is in the 4th unit test (specifically constructed for that, suggested by @instagibbs).
I am considering a simplification for the encoding format howe
...
🤔 furszy reviewed a pull request: "wallet: optimize migration process, batch db transactions"
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2157536398)
> In ApplyMigrationData, it looks like we're creating batchs for the watchonly and solvable wallets multiple times. Those could be consolidated in this PR as well.
Done. Thanks.
(https://github.com/bitcoin/bitcoin/pull/28574#pullrequestreview-2157536398)
> In ApplyMigrationData, it looks like we're creating batchs for the watchonly and solvable wallets multiple times. Those could be consolidated in this PR as well.
Done. Thanks.
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1664854646)
Done
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1664854646)
Done
💬 furszy commented on pull request "wallet: optimize migration process, batch db transactions":
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1664855357)
Done
(https://github.com/bitcoin/bitcoin/pull/28574#discussion_r1664855357)
Done
🤔 pablomartin4btc reviewed a pull request: "Testnet4 including PoW difficulty adjustment fix"
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2157550766)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/29775#pullrequestreview-2157550766)
Concept ACK
👍 hodlinator approved a pull request: "Several randomness improvements"
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2154709229)
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
Mainly code review. Did not attempt to check thread safety compliance. (Passed `make check` & `test/functional/test_runner.py`).
### e2d1f84858485650ff743753ffa5c679f210a992 - random: make GetRand() support entire range (incl. max)
The commit message title probably should be "random: make GetRand\<T\>() support entire range (incl. max)", since the overloads taking parameters still are exclusive at the end. It felt dangerous that `GetRand<T>(v
...
(https://github.com/bitcoin/bitcoin/pull/29625#pullrequestreview-2154709229)
ACK ce8094246ee95232e9d84f7e37f3c0a43ef587ce
Mainly code review. Did not attempt to check thread safety compliance. (Passed `make check` & `test/functional/test_runner.py`).
### e2d1f84858485650ff743753ffa5c679f210a992 - random: make GetRand() support entire range (incl. max)
The commit message title probably should be "random: make GetRand\<T\>() support entire range (incl. max)", since the overloads taking parameters still are exclusive at the end. It felt dangerous that `GetRand<T>(v
...
💬 hodlinator commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664692692)
In d5fcbe966bc501db8bf6a3809633f0b82e6ae547: Always throwing away 11 bits of entropy in the new version compared to 0 before the PR. I guess you want to preserve the expression from the linked site and it's unclear whether not throwing away the bits would be more performant.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664692692)
In d5fcbe966bc501db8bf6a3809633f0b82e6ae547: Always throwing away 11 bits of entropy in the new version compared to 0 before the PR. I guess you want to preserve the expression from the linked site and it's unclear whether not throwing away the bits would be more performant.
💬 hodlinator commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1663070564)
In 21ce9d8658fed0d3e4552e8b02a6902cb31c572e: nit: typo - "bits bits"
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1663070564)
In 21ce9d8658fed0d3e4552e8b02a6902cb31c572e: nit: typo - "bits bits"
💬 hodlinator commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664818014)
In e2d1f84858485650ff743753ffa5c679f210a992: Now we are in C++20 land, it might be time to use real designated initializers instead of comments when touching blocks like these?
```C++
.id=id,
.m_connected=std::chrono::seconds{random_context.randrange(100)},
.m_min_ping_time=std::chrono::microseconds{random_context.randrange(100)},
.m_last_block_time=std::chrono::seconds{random_context.randrange(100)},
.m_last_tx_time=std::chrono::
...
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664818014)
In e2d1f84858485650ff743753ffa5c679f210a992: Now we are in C++20 land, it might be time to use real designated initializers instead of comments when touching blocks like these?
```C++
.id=id,
.m_connected=std::chrono::seconds{random_context.randrange(100)},
.m_min_ping_time=std::chrono::microseconds{random_context.randrange(100)},
.m_last_block_time=std::chrono::seconds{random_context.randrange(100)},
.m_last_tx_time=std::chrono::
...
💬 hodlinator commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664856842)
In ddc184d999d7e1a87efaf6bcb222186f0dcd87ec: The commit message in e2d1f84858485650ff743753ffa5c679f210a992 made me do a survey of (mis)uses of `GetRand()` . 2 similar cases stood out to me at first, but they appear correct after some noodling. This is one of them and the other is `check_block_index` in validation.cpp.
(`check_ratio` is often set to `1` (always) or `0` (never)).
Sharing the same `return` path actually makes the behavior slightly quicker for me to grok:
```C++
if (m_o
...
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664856842)
In ddc184d999d7e1a87efaf6bcb222186f0dcd87ec: The commit message in e2d1f84858485650ff743753ffa5c679f210a992 made me do a survey of (mis)uses of `GetRand()` . 2 similar cases stood out to me at first, but they appear correct after some noodling. This is one of them and the other is `check_block_index` in validation.cpp.
(`check_ratio` is often set to `1` (always) or `0` (never)).
Sharing the same `return` path actually makes the behavior slightly quicker for me to grok:
```C++
if (m_o
...
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664868589)
It's not actually a typo, it means "Return the bottom `bits` many bits", but I do see why it'd confusing. Will address if I have to repush.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664868589)
It's not actually a typo, it means "Return the bottom `bits` many bits", but I do see why it'd confusing. Will address if I have to repush.
💬 sipa commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664869992)
Yeah, I wanted to keep `MakeExponential` as stand-alone reviewable as possible. I don't think any of the call sites are so performance-critical that the difference matters.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1664869992)
Yeah, I wanted to keep `MakeExponential` as stand-alone reviewable as possible. I don't think any of the call sites are so performance-critical that the difference matters.
💬 fjahr commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207407809)
> I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?
That appears to be a bug. We actually require the mempool to be empty so it not being there should not be a problem. I will open a PR.
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207407809)
> I also removed the mempool and it crashed when reaching validation.cpp:5684. Is it a hard requirement when using assumeutxo that a mempool exists, or is this a bug?
That appears to be a bug. We actually require the mempool to be empty so it not being there should not be a problem. I will open a PR.