Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restore full functionality to trim_headers (untrim) #1270

Merged
merged 19 commits into from
Jun 6, 2024

Conversation

psgreco
Copy link
Contributor

@psgreco psgreco commented Oct 2, 2023

Currently, trim_headers has certain limitations wrt the way it behaves in P2P protocol and in certain RPC calls. This PR brings back the full functionality and fixes a corruption issue when combined with pruning.

Fixes: #1241

@@ -5,6 +5,11 @@

#include <chain.h>
#include <util/time.h>
#include <validation.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a circular dependency.

"chain.h -> validation.h -> chain.h" 

I'm not quite sure how to fix it -- we can't simply forward declare ChainManager here since the line below requires knowledge of the fields of ChainManager:

m_pcontext->chainman->m_blockman.m_dirty_blockindex.insert(this);

@@ -1710,7 +1711,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)

// if pruning, unset the service bit and perform the initial blockstore prune
// after any wallet rescanning has taken place.
if (fPruneMode || node::fTrimHeaders) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was || node::fTrimHeaders removed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conditional is unsetting NODE_NETWORK, without this flag enabled it means this peer can't serve historical block data (ie. it's a pruned node). This was previously necessary because we weren't able to "untrim" the headers, which this PR fixes.

Copy link
Member

@delta1 delta1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK eed85dc

I added the trim headers test from #1249 and confirmed that it passed here but fails on master (fixing #1334)

No noticeable effects during IBD from the additional locking.

@whitslack
Copy link

I have been running a pruned Elements node with this patch continuously for months and have seen no issues at all.

That said, memory usage is still unacceptably high at 2.3 GiB. Keeping the entire chain of block header hashes, going back to the genesis block, in RAM at all times is unsustainable, especially since a new block is added to the Liquid chain every minute. It means elementsd is almost entirely swapped out on my system — I see 20 MiB RSS and 2368 MiB in swap — and unfortunately elementsd accesses all of those pages during its shutdown, which means that my system takes many minutes to shut down, not great when the shutdown is triggered by UPS batteries nearing exhaustion. I know Elements is just copying what Bitcoin Core does, but they have one-tenth the block generation rate, so the issue is not as bad there, although it is increasingly becoming a problem over time. There has got to be a better way.

@psgreco
Copy link
Contributor Author

psgreco commented Jun 6, 2024

UTACK eed85dc on all the additions.

@delta1 delta1 merged commit cdcc74b into ElementsProject:master Jun 6, 2024
9 of 13 checks passed
@psgreco psgreco deleted the master-trim-headers-v2 branch June 6, 2024 13:31
@psgreco
Copy link
Contributor Author

psgreco commented Jun 6, 2024

I have been running a pruned Elements node with this patch continuously for months and have seen no issues at all.

That said, memory usage is still unacceptably high at 2.3 GiB. Keeping the entire chain of block header hashes, going back to the genesis block, in RAM at all times is unsustainable, especially since a new block is added to the Liquid chain every minute. It means elementsd is almost entirely swapped out on my system — I see 20 MiB RSS and 2368 MiB in swap — and unfortunately elementsd accesses all of those pages during its shutdown, which means that my system takes many minutes to shut down, not great when the shutdown is triggered by UPS batteries nearing exhaustion. I know Elements is just copying what Bitcoin Core does, but they have one-tenth the block generation rate, so the issue is not as bad there, although it is increasingly becoming a problem over time. There has got to be a better way.

Hey, yeap, I agree that we need to continue reducing the usage, but this is the most we can do without diverging from what bitcoind does, any change after that would have to be much more invasive

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crashes after being off for a few days
4 participants