💬 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.
💬 mzumsande commented on issue "Assertion chainman().ActiveChain()[height] fails on startup on memory-constrained system":
(https://github.com/bitcoin/bitcoin/issues/29422#issuecomment-2207413826)
I could reproduce this, both on 26.0 and master, by simlulating the oom by adding the assert **after** the block index was loaded.
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index fe466fe5aa..352c802576 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2891,6 +2891,7 @@ bool Chainstate::FlushStateToDisk(
if (!m_blockman.WriteBlockIndexDB()) {
return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to bl
...
(https://github.com/bitcoin/bitcoin/issues/29422#issuecomment-2207413826)
I could reproduce this, both on 26.0 and master, by simlulating the oom by adding the assert **after** the block index was loaded.
```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index fe466fe5aa..352c802576 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2891,6 +2891,7 @@ bool Chainstate::FlushStateToDisk(
if (!m_blockman.WriteBlockIndexDB()) {
return FatalError(m_chainman.GetNotifications(), state, _("Failed to write to bl
...
💬 tdb3 commented on pull request "net: Allow -proxy=[::1] on nodes with IPV6 lo only":
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1664879365)
Yeah, it would. The thought here is to increase resilience in the code, so that if ai_flags changes we're not hard-coded to 0. Seems like the scope of this `if` condition is intended as "if AI_ADDRCONFIG set" rather than "is only AI_ADDRCONFIG set" (which is now updated in f95fb4a1d826765553f3319ee6f1024094954073). I'm thinking we would want `callgetaddrinfo()` to use similar logic (i.e. keep flags defined except for AI_ADDRCONFIG).
(https://github.com/bitcoin/bitcoin/pull/30245#discussion_r1664879365)
Yeah, it would. The thought here is to increase resilience in the code, so that if ai_flags changes we're not hard-coded to 0. Seems like the scope of this `if` condition is intended as "if AI_ADDRCONFIG set" rather than "is only AI_ADDRCONFIG set" (which is now updated in f95fb4a1d826765553f3319ee6f1024094954073). I'm thinking we would want `callgetaddrinfo()` to use similar logic (i.e. keep flags defined except for AI_ADDRCONFIG).
👍 cbergqvist approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2157640758)
ACK bca346a97056748f1e3fb899f336d56d9fd45a64
>> The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor
> Only tests that use `bind_to_localhost_only` will do. We have 5 such tests.
We have 5 tests that *disable* `bind_to_localhost_only`, all the others will `bind=127.0.0.1`. Having that will result in your new `has_explicit_bind` being true for the majority of tests, which will lead to your
...
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2157640758)
ACK bca346a97056748f1e3fb899f336d56d9fd45a64
>> The current solution means most test nodes in the functional test suite will now bind to an onion port even though the vast majority are not testing Tor
> Only tests that use `bind_to_localhost_only` will do. We have 5 such tests.
We have 5 tests that *disable* `bind_to_localhost_only`, all the others will `bind=127.0.0.1`. Having that will result in your new `has_explicit_bind` being true for the majority of tests, which will lead to your
...
💬 fjahr commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207510865)
> > 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.
This should work https://github.com/fjahr/bitcoin/commit/21530b489eee6b8db4b9af45430d9d3cb43a459c, but on second thought I am not sure if we need this (yet). Conceptually it makes sense
...
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2207510865)
> > 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.
This should work https://github.com/fjahr/bitcoin/commit/21530b489eee6b8db4b9af45430d9d3cb43a459c, but on second thought I am not sure if we need this (yet). Conceptually it makes sense
...
📝 theuni opened a pull request: "contrib: use c++ compiler rather than c compiler for binary checks"
(https://github.com/bitcoin/bitcoin/pull/30387)
See discussion here: https://github.com/hebasto/bitcoin/pull/252#discussion_r1664657488
Use CXX/CXXFLAGS rather than CC/CFLAGS to test our actual compiler for binary checks rather than the one we only forward to secp256k1.
(https://github.com/bitcoin/bitcoin/pull/30387)
See discussion here: https://github.com/hebasto/bitcoin/pull/252#discussion_r1664657488
Use CXX/CXXFLAGS rather than CC/CFLAGS to test our actual compiler for binary checks rather than the one we only forward to secp256k1.
👍 tdb3 approved a pull request: "Update manpage descriptions"
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2157725238)
re ACK f6171a8f1dabc155c47faf9a67ef87dea436c623
Re-ran actions in previous review https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-1956428566
(https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-2157725238)
re ACK f6171a8f1dabc155c47faf9a67ef87dea436c623
Re-ran actions in previous review https://github.com/bitcoin/bitcoin/pull/29686#pullrequestreview-1956428566