💬 Sjors commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324677976)
The `invalidateblock` -> `reconsiderblock` block approach doesn't work, because the timewarp attack is only checked in `ContextualCheckBlockHeader`. According to the code documentation `-reindex-chainstate` won't work either.
`bitcoind -reindex` does the trick, and only takes a minute on this tiny chain fortunately.
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324677976)
The `invalidateblock` -> `reconsiderblock` block approach doesn't work, because the timewarp attack is only checked in `ContextualCheckBlockHeader`. According to the code documentation `-reindex-chainstate` won't work either.
`bitcoind -reindex` does the trick, and only takes a minute on this tiny chain fortunately.
💬 fanquake commented on issue "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time":
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324681538)
It's actually irrelevant for all platforms other than windows with vcpkg (as far as I'm aware), and was added to work around how boost is packaged there (we didn't have an equivalent in autotools). It should be scooped as such, rather than making all other platforms run the (pointless) slow check.
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324681538)
It's actually irrelevant for all platforms other than windows with vcpkg (as far as I'm aware), and was added to work around how boost is packaged there (we didn't have an equivalent in autotools). It should be scooped as such, rather than making all other platforms run the (pointless) slow check.
💬 Sjors commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324687262)
cc @mempool
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324687262)
cc @mempool
💬 maflcko commented on issue "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time":
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324693670)
> Verifying that the header simply exists could be an alternative.
Yeah, that seems ideal, given that this was done for autotools as well, assuming it also works with vcpkg?
> It compiles and links the minimal code that uses the `boost/test/included/unit_test.hpp` header.
I am using ccache to compile, but I guess this won't be picked up here?
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324693670)
> Verifying that the header simply exists could be an alternative.
Yeah, that seems ideal, given that this was done for autotools as well, assuming it also works with vcpkg?
> It compiles and links the minimal code that uses the `boost/test/included/unit_test.hpp` header.
I am using ccache to compile, but I guess this won't be picked up here?
💬 fanquake commented on issue "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time":
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324700285)
> Yeah, that seems ideal, given that this was done for autotools as well,
Where in Autotools were we running checks to verify the existence of singlular boost headers at configure time?
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324700285)
> Yeah, that seems ideal, given that this was done for autotools as well,
Where in Autotools were we running checks to verify the existence of singlular boost headers at configure time?
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2324700313)
The test runs unreliably for some reason, we need to fix that first.
NACK 5cad9d16dd07859a76c6637789695bf1b4f36e1c
> On this branch [...] main thread for 115 milliseconds.
This sounds awesome!
> Good point, I can add a test that that expects flush every 24 hours, then reduce it to 1 hour after this change.
Do you think we could still do something like this, i.e. add a test as a very first commit, which reproduces the old behavior first?
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2324700313)
The test runs unreliably for some reason, we need to fix that first.
NACK 5cad9d16dd07859a76c6637789695bf1b4f36e1c
> On this branch [...] main thread for 115 milliseconds.
This sounds awesome!
> Good point, I can add a test that that expects flush every 24 hours, then reduce it to 1 hour after this change.
Do you think we could still do something like this, i.e. add a test as a very first commit, which reproduces the old behavior first?
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740812194)
The initial run should never flush, right?
`SetMockTime(GetTime<std::chrono::minutes>() + 55min);` passes also, so we should likely update the test
.
Actually, sometimes it passes, other times it fails - this is the reason for my NACK
```
% build/src/test/test_bitcoin --run_test=chainstate_write_tests/chainstate_write_interval
Running 1 test case...
src/test/chainstate_write_tests.cpp:35: error: in "chainstate_write_tests/chainstate_write_interval": check !sub->m_did_flush has fai
...
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740812194)
The initial run should never flush, right?
`SetMockTime(GetTime<std::chrono::minutes>() + 55min);` passes also, so we should likely update the test
.
Actually, sometimes it passes, other times it fails - this is the reason for my NACK
```
% build/src/test/test_bitcoin --run_test=chainstate_write_tests/chainstate_write_interval
Running 1 test case...
src/test/chainstate_write_tests.cpp:35: error: in "chainstate_write_tests/chainstate_write_interval": check !sub->m_did_flush has fai
...
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740868062)
Besides the stability and the initial run, I think we should test the valid values as well inside the range, e.g.
```C++
BOOST_FIXTURE_TEST_CASE(chainstate_write_interval, TestingSetup)
{
auto DATABASE_WRITE_INTERVAL_MIN{50min};
auto DATABASE_WRITE_INTERVAL_MAX{70min};
auto now = GetTime<std::chrono::minutes>();
auto sub{std::make_shared<TestSubscriber>()};
m_node.validation_signals->RegisterSharedValidationInterface(sub);
auto test_flush = [&](std::chrono::minut
...
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740868062)
Besides the stability and the initial run, I think we should test the valid values as well inside the range, e.g.
```C++
BOOST_FIXTURE_TEST_CASE(chainstate_write_interval, TestingSetup)
{
auto DATABASE_WRITE_INTERVAL_MIN{50min};
auto DATABASE_WRITE_INTERVAL_MAX{70min};
auto now = GetTime<std::chrono::minutes>();
auto sub{std::make_shared<TestSubscriber>()};
m_node.validation_signals->RegisterSharedValidationInterface(sub);
auto test_flush = [&](std::chrono::minut
...
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740870587)
can we add @sipa's reasoning here for why it's not a fixed value?
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740870587)
can we add @sipa's reasoning here for why it's not a fixed value?
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740880618)
If my understanding is correct:
* at startup we never want to flush, but the initial value of `m_next_write` would trigger one, so we have to set the next write value in that case quickly to prevent one
* when we do flush, we want to set the next flush's time again
I think we can deduplicate setting this value twice (which also enables us to inline `CalculateNextWrite`):
* setting m_next_write to max, which won't trigger `fDoFullFlush` since it's `>> nNow`
* add another boolean where we c
...
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740880618)
If my understanding is correct:
* at startup we never want to flush, but the initial value of `m_next_write` would trigger one, so we have to set the next write value in that case quickly to prevent one
* when we do flush, we want to set the next flush's time again
I think we can deduplicate setting this value twice (which also enables us to inline `CalculateNextWrite`):
* setting m_next_write to max, which won't trigger `fDoFullFlush` since it's `>> nNow`
* add another boolean where we c
...
💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740758502)
I was a bit hasty yesterday - the MIN/MAX naming indicates that the interval is closed (i.e. can actually reach the values), so we should add 1 minute to the range:
```suggestion
NodeClock::time_point CalculateNextWrite(const NodeClock::time_point after)
{
const auto time{after + DATABASE_WRITE_INTERVAL_MIN};
constexpr auto range{DATABASE_WRITE_INTERVAL_MAX - DATABASE_WRITE_INTERVAL_MIN + 1min};
return FastRandomContext().rand_uniform_delay(time, range);
}
```
See:
```C++
...
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740758502)
I was a bit hasty yesterday - the MIN/MAX naming indicates that the interval is closed (i.e. can actually reach the values), so we should add 1 minute to the range:
```suggestion
NodeClock::time_point CalculateNextWrite(const NodeClock::time_point after)
{
const auto time{after + DATABASE_WRITE_INTERVAL_MIN};
constexpr auto range{DATABASE_WRITE_INTERVAL_MAX - DATABASE_WRITE_INTERVAL_MIN + 1min};
return FastRandomContext().rand_uniform_delay(time, range);
}
```
See:
```C++
...
💬 fanquake commented on issue "DNS seed "seed.bitcoinstats.com" doesn't support filtering while the comments says it does":
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2324717789)
> What would be the best way for that?
I think a new PR for re-adding is fine.
> And more importantly, do we even need / want an additional seeder?
Opening an issue to gather opinions is probably the best way to have this discussion.
(https://github.com/bitcoin/bitcoin/issues/29911#issuecomment-2324717789)
> What would be the best way for that?
I think a new PR for re-adding is fine.
> And more importantly, do we even need / want an additional seeder?
Opening an issue to gather opinions is probably the best way to have this discussion.
💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2324722478)
> It is used in your change from
BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);
I don't think it's related, commenting out:
```diff
-inline std::ostream& operator<<(std::ostream& os, const std::nullopt_t)
-{
- return os << "std::nullopt";
-}
-
-template <typename T>
-inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
-{
- if (v) {
- return os << v.value();
- } else {
- return os << std::nullopt;
- }
-}
+
...
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2324722478)
> It is used in your change from
BOOST_CHECK_EQUAL(uint256::FromUserHex("").value(), uint256::ZERO);
I don't think it's related, commenting out:
```diff
-inline std::ostream& operator<<(std::ostream& os, const std::nullopt_t)
-{
- return os << "std::nullopt";
-}
-
-template <typename T>
-inline std::ostream& operator<<(std::ostream& os, const std::optional<T>& v)
-{
- if (v) {
- return os << v.value();
- } else {
- return os << std::nullopt;
- }
-}
+
...
💬 Sjors commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324725014)
> so I am now just using the python script from cmake as well, leaving us with just the python implementation
Mmm, is Python already a build requirement though?
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324725014)
> so I am now just using the python script from cmake as well, leaving us with just the python implementation
Mmm, is Python already a build requirement though?
💬 maflcko commented on issue "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time":
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324729606)
> Where in Autotools were we running checks to verify the existence of singlular boost headers at configure time?
I haven't confirmed this, but I assumed it checked for `boost/version.hpp` existence (and version conformance). Though, maybe I am wrong. In any case, anything is fine by me. I just found that it would be nice if calling `rm -rf ./bld-cmake/ && cmake -B ./bld-cmake` was fast, so all developers can just call it every time, without having to think about it much.
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324729606)
> Where in Autotools were we running checks to verify the existence of singlular boost headers at configure time?
I haven't confirmed this, but I assumed it checked for `boost/version.hpp` existence (and version conformance). Though, maybe I am wrong. In any case, anything is fine by me. I just found that it would be nice if calling `rm -rf ./bld-cmake/ && cmake -B ./bld-cmake` was fast, so all developers can just call it every time, without having to think about it much.
💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740907250)
but we're never writing optionals on the right side here, i.e.
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), std::make_optional(uint256::ZERO));
```
can simply be either:
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);
```
or if empty:
```C++
BOOST_CHECK(!uint256::FromUserHex(""));
```
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740907250)
but we're never writing optionals on the right side here, i.e.
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), std::make_optional(uint256::ZERO));
```
can simply be either:
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);
```
or if empty:
```C++
BOOST_CHECK(!uint256::FromUserHex(""));
```
📝 jadijadi opened a pull request: "test: fixing failing system_tests/run_command under some Locales"
(https://github.com/bitcoin/bitcoin/pull/30788)
the run_command test under system_tests fails if the locale is anything other than US/UK/C (English ones)
because it checks for things like `result.find_value("success");`
To prevent this, a `setenv("LC_ALL", "C", 1);` is added to make sure that the tests are being run under C locale even if something else is set in the terminal.
fixes #30608
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
...
(https://github.com/bitcoin/bitcoin/pull/30788)
the run_command test under system_tests fails if the locale is anything other than US/UK/C (English ones)
because it checks for things like `result.find_value("success");`
To prevent this, a `setenv("LC_ALL", "C", 1);` is added to make sure that the tests are being run under C locale even if something else is set in the terminal.
fixes #30608
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
...
💬 marcofleon commented on pull request "doc: fix compiler flags for macOS configuration":
(https://github.com/bitcoin/bitcoin/pull/30785#discussion_r1740908329)
The word "confgure" is still used in all the build docs to describe that step (not just mac), so I'll leave that as is. Maybe another PR can reword the descriptions if we want to do that.
(https://github.com/bitcoin/bitcoin/pull/30785#discussion_r1740908329)
The word "confgure" is still used in all the build docs to describe that step (not just mac), so I'll leave that as is. Maybe another PR can reword the descriptions if we want to do that.
💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740911993)
Removed the `nullopt` overwrite and explained the usage in the comment, thanks for explaining your view.
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740911993)
Removed the `nullopt` overwrite and explained the usage in the comment, thanks for explaining your view.
💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324740342)
> if the header can only be generated using a Python script, then that's an argument for including it in the source code.
It is possible to generate it in native cmake as well. See for example `cmake/script/GenerateHeaderFromJson.cmake`. Note that previously the "cmake" generation used `xxd`, which isn't a build-requirement either.
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324740342)
> if the header can only be generated using a Python script, then that's an argument for including it in the source code.
It is possible to generate it in native cmake as well. See for example `cmake/script/GenerateHeaderFromJson.cmake`. Note that previously the "cmake" generation used `xxd`, which isn't a build-requirement either.