💬 TheCharlatan commented on pull request "[refactor] Cleanup BlockAssembler mempool usage":
(https://github.com/bitcoin/bitcoin/pull/28843#issuecomment-2324628138)
Reworked a323388036d4f8b9fe8ff0915252f20abb91633e -> 247ae848e669cc36c790362b766ff813cdfa3e66 ([blockAssemblerRemoveMempool_1](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_1) -> [blockAssemblerRemoveMempool_2](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockAssemblerRemoveMempool_1..blockAssemblerRemoveMempool_2))
* Took @ajtowns's [suggestion](https://github.com/bitcoin/bit
...
(https://github.com/bitcoin/bitcoin/pull/28843#issuecomment-2324628138)
Reworked a323388036d4f8b9fe8ff0915252f20abb91633e -> 247ae848e669cc36c790362b766ff813cdfa3e66 ([blockAssemblerRemoveMempool_1](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_1) -> [blockAssemblerRemoveMempool_2](https://github.com/TheCharlatan/bitcoin/tree/blockAssemblerRemoveMempool_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockAssemblerRemoveMempool_1..blockAssemblerRemoveMempool_2))
* Took @ajtowns's [suggestion](https://github.com/bitcoin/bit
...
💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740836897)
Using a mutable C-style array seems a bit fragile. IIUC, starting with C++17, using `inline constexpr std::array ip_asn{...};` should allow the compiler to optimize the layout, as well as enforce immutability. Moreover, `std::array` drops the need for `ip_asn_len`, because the size is embedded in the type of the array. Unrelated, you can change the `0x{...}` above to `std::byte{...}` and then use `MakeUCharSpan` instead of legacy C-Style reinterpret cast and static cast in the code that uses the
...
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1740836897)
Using a mutable C-style array seems a bit fragile. IIUC, starting with C++17, using `inline constexpr std::array ip_asn{...};` should allow the compiler to optimize the layout, as well as enforce immutability. Moreover, `std::array` drops the need for `ip_asn_len`, because the size is embedded in the type of the array. Unrelated, you can change the `0x{...}` above to `std::byte{...}` and then use `MakeUCharSpan` instead of legacy C-Style reinterpret cast and static cast in the code that uses the
...
⚠️ Sjors opened an issue: "Testnet4 consensus failure due to timewarp related "softfork""
(https://github.com/bitcoin/bitcoin/issues/30786)
#30647 was effectively a soft-fork on testnet4 with no activation plan.
It reduced the timewarp mitigation allowance from 7200 to 600 seconds.
It's enforced by folks running v28.0rc1 and recent master, but not by miners.
This causes new nodes to be stuck at block 42,335: https://mempool.space/testnet4/block/00000000542792e54a720567ba66157d48cdae7bfd01c1b678d0f07a2ed56e99
As observed on BitcoinTalk: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234 and pointed ou
...
(https://github.com/bitcoin/bitcoin/issues/30786)
#30647 was effectively a soft-fork on testnet4 with no activation plan.
It reduced the timewarp mitigation allowance from 7200 to 600 seconds.
It's enforced by folks running v28.0rc1 and recent master, but not by miners.
This causes new nodes to be stuck at block 42,335: https://mempool.space/testnet4/block/00000000542792e54a720567ba66157d48cdae7bfd01c1b678d0f07a2ed56e99
As observed on BitcoinTalk: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234 and pointed ou
...
💬 Sjors commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324655223)
Note that the block that upgraded nodes will get stuck on (42,335) has difficulty 1. It was probably mined by setting the machine clock 20 minutes in the future. The original testnet4 rules allowed the next block to use the real time. But under the new rules it needs to be 10 minutes in the future (which #30681 takes care of).
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2324655223)
Note that the block that upgraded nodes will get stuck on (42,335) has difficulty 1. It was probably mined by setting the machine clock 20 minutes in the future. The original testnet4 rules allowed the next block to use the real time. But under the new rules it needs to be 10 minutes in the future (which #30681 takes care of).
⚠️ maflcko opened an issue: "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time"
(https://github.com/bitcoin/bitcoin/issues/30787)
It looks like this step is taking longer than the entire `cmake -B` without it. Is there a reason why it takes so long, an whether it can be sped up?
(https://github.com/bitcoin/bitcoin/issues/30787)
It looks like this step is taking longer than the entire `cmake -B` without it. Is there a reason why it takes so long, an whether it can be sped up?
💬 hebasto 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-2324671298)
It compiles and links the minimal code that uses the `boost/test/included/unit_test.hpp` header. Verifying that the header simply exists could be an alternative.
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324671298)
It compiles and links the minimal code that uses the `boost/test/included/unit_test.hpp` header. Verifying that the header simply exists could be an alternative.
💬 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?