🤔 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
💬 maflcko commented on pull request "fuzz: improve utxo_snapshot target":
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2208193267)
> 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.
Nice. Would be good to have a pull request with this, so that it can be reviewed and merged.
(https://github.com/bitcoin/bitcoin/pull/30329#issuecomment-2208193267)
> 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.
Nice. Would be good to have a pull request with this, so that it can be reviewed and merged.
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665187839)
This is already a list initialization, so I don't think clang-tidy can pick up the named args at all. Happy to review a follow-up, if you decide to open one.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665187839)
This is already a list initialization, so I don't think clang-tidy can pick up the named args at all. Happy to review a follow-up, if you decide to open one.
💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665189772)
I am happy to review a follow-up, if you decide to open one.
(https://github.com/bitcoin/bitcoin/pull/29625#discussion_r1665189772)
I am happy to review a follow-up, if you decide to open one.
💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2208225151)
Rebased after #30324.
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2208225151)
Rebased after #30324.
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1665208121)
This is copied from `GetNextWorkRequired` which doesn't have the `nHeight` helper. The simplification makes sense, or we can add a helper function: https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1654288556
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1665208121)
This is copied from `GetNextWorkRequired` which doesn't have the `nHeight` helper. The simplification makes sense, or we can add a helper function: https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1654288556