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

main, wire, blockchain, indexers, ffldb: Add pruning #1971

Merged
merged 14 commits into from
Aug 23, 2023

Conversation

kcalvinalvin
Copy link
Collaborator

@kcalvinalvin kcalvinalvin commented Apr 28, 2023

Adds pruning to btcd.

Change to ffldb:

  1. Add PruneBlocks() to transaction interface. Prune blocks is a no-op when the total block file size is equal to or under the target size. If the total block files size is over the target size, oldest block files are deleted along with the block indexes that point to those block files. Returns the hashes of the deleted blocks.

Change to wire:

  1. Add SFNodeNetworkLimited.
  2. Add SFNodeNetworkLimited to default services.

Change to blockchain:

  1. New pruneTarget param to indicate how much the block files should take up.l
  2. Attempts to prune blocks, spend journals, and relevant indexes on every connectBlock

Change to indexers:

  1. Added PruneBlock to Manager and implemented PruneBlock to every indexer.

Change to main:

  1. Prune flag is defined. It limits the prune to 1536MiB as we need to guarantee 288 blocks to abide by the NODE_NETWORK_LIMITED rule set by BIP159.
  2. --addrindex and --prune at the same time is not allowed.
  3. --txindex and --prune at the same time is not allowed.

Fixes #1069

@kcalvinalvin kcalvinalvin mentioned this pull request Apr 28, 2023
@coveralls
Copy link

coveralls commented Apr 28, 2023

Pull Request Test Coverage Report for Build 5936174185

  • 85 of 252 (33.73%) changed or added relevant lines in 11 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.09%) to 55.28%

Changes Missing Coverage Covered Lines Changed/Added Lines %
server.go 0 4 0.0%
blockchain/indexers/addrindex.go 0 8 0.0%
blockchain/indexers/cfindex.go 0 8 0.0%
blockchain/indexers/txindex.go 0 8 0.0%
blockchain/chainio.go 0 9 0.0%
rpcserver.go 0 9 0.0%
database/ffldb/blockio.go 25 35 71.43%
blockchain/chain.go 4 16 25.0%
database/ffldb/db.go 56 76 73.68%
config.go 0 21 0.0%
Files with Coverage Reduction New Missed Lines %
rpcserver.go 4 0.26%
Totals Coverage Status
Change from base Build 5872394378: -0.09%
Covered Lines: 26925
Relevant Lines: 48707

💛 - Coveralls

@saubyk
Copy link

saubyk commented May 4, 2023

cc @halseth in case you're interested in reviewing this pr. Thanks

Copy link
Collaborator

@halseth halseth left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me :)

database/ffldb/blockio.go Outdated Show resolved Hide resolved
database/ffldb/blockio.go Outdated Show resolved Hide resolved
database/ffldb/db.go Outdated Show resolved Hide resolved
database/interface.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Great work, very cool to see such an impactful feature being introduced in only 8 commits! Did a first pass review, will do some manual testing in the next round.

database/ffldb/blockio.go Outdated Show resolved Hide resolved
database/ffldb/blockio.go Outdated Show resolved Hide resolved
database/ffldb/blockio.go Outdated Show resolved Hide resolved
database/ffldb/db.go Outdated Show resolved Hide resolved
database/ffldb/db.go Outdated Show resolved Hide resolved
blockchain/chain.go Outdated Show resolved Hide resolved
@@ -255,6 +255,37 @@ func (idx *CfIndex) DisconnectBlock(dbTx database.Tx, block *btcutil.Block,
return nil
}

// PruneBlock is invoked when an older block is deleted after it's been
// processed. For cfindx, PruneBlock is exactly the same as disconnecting a block.
// TODO (kcalvinalvin): Consider keeping the filters at a later date to help with
Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess we could also support fast rescan using compact filters in the future when pruning? Maybe it would be a great idea to have an option like prune-cfilters=true/false that is false by default (since filters for mainnet are currently only around 8 GB).

Copy link
Collaborator Author

@kcalvinalvin kcalvinalvin Jun 1, 2023

Choose a reason for hiding this comment

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

I just realized that the fast rescan for bitcoin core is for the wallet. Since btcd doesn't have any wallet functionality with itself, there isn't a need for the compact filters for this reason.

Does btcwallet ask for the filters(not familiar with the codebase)? Might be worth keeping the filters around for it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, you're right. I keep forgetting that btcd doesn't have its own wallet. Not sure what the user expectation would be then. But I assume if you are serving the network (or your own Neutrino based lnd) you might not turn on pruning in the first place. So I think it's okay to keep that as a TODO for now.

Copy link
Member

Choose a reason for hiding this comment

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

Does btcwallet ask for the filters(not familiar with the codebase)? Might be worth keeping the filters around for it?

We don't do that yet but it's something we've been meaning on adding.

wire/protocol.go Show resolved Hide resolved
@@ -66,6 +66,7 @@ const (
sampleConfigFilename = "sample-btcd.conf"
defaultTxIndex = false
defaultAddrIndex = false
pruneMinSize = 1536
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I see there's another min size here. Shouldn't we sync this with what's being checked in database/ffldb/db.go?

Copy link
Collaborator Author

@kcalvinalvin kcalvinalvin May 31, 2023

Choose a reason for hiding this comment

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

So the 1536 number came from the fact that we have these constants.

blocksNeeded = 288
ConsensusMaxBlockSize = 4,000,000
onDiskBlkFileSizeInBytes = 536,870,912

We need to keep 288 blocks to adhere to NODE_NETWORK_LIMITED BIP. Each block can be a maximum of 4MB. Each block file (XXXXXX.fdb) is 512MiB.

(blocksNeeded*ConsensusMaxBlockSize)/onDiskBlkFileSizeInBytes = 2.145

We do the math and we need to keep 2.14 on disk .fdb files to adhere to the NODE_NETWORK_LIMITED rule. Can't keep .14 files so we round up to 3. 512MiB * 3 is 1536MiB which is our pruneMinSize.

As mentioned here, the check in database/ffldb/db.go is a different check so it's ok.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this correctly it also seems like the smallest resolution we can target is 512MiB increments which means that maybe our settings should use GiB as the unit of measurement for pruning's sake. Right now it specifies it with a uint32 of bytes, which practically means that there are effectively 6 configurations that can come out of this: 1.5, 2.0, 2.5, 3.0, 3.5, and 4.0 GiB?

I'm interpreting this because you said we can't keep partial block files so that means that we effectively only have the discrete intervals of 512MiB to work with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Each blk file is 512MiB and while we can delete parts of the file, it's not efficient to do so and won't have much user benefit.

It's the same as Core where you give a target and core deletes .dat files to meet that target. It'll be in those increments you state but the user is free to give any amount (just like in Core).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I don't mind the intervals at all. It's hardly an issue now that the overall prune cache size isn't maxed at ~4G

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah okay, the two different limits now make sense to me, thanks for the explanation.

config.go Outdated Show resolved Hide resolved
@kcalvinalvin kcalvinalvin force-pushed the add-pruning branch 3 times, most recently from ac5d860 to 30522d2 Compare June 1, 2023 08:35
@guggero guggero self-requested a review June 15, 2023 14:30
Copy link
Contributor

@ProofOfKeags ProofOfKeags left a comment

Choose a reason for hiding this comment

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

There are a couple of things I think are necessary to change like the Prune Cache uint width. There are a number of things that seem fishy to me architecturally but nothing I can point to that's broken about it. I think even with the architectural smells that the benefit of a prunable btcd is worth merging these changes.

blockchain/chain.go Outdated Show resolved Hide resolved
blockchain/chain.go Outdated Show resolved Hide resolved
@@ -66,6 +66,7 @@ const (
sampleConfigFilename = "sample-btcd.conf"
defaultTxIndex = false
defaultAddrIndex = false
pruneMinSize = 1536
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading this correctly it also seems like the smallest resolution we can target is 512MiB increments which means that maybe our settings should use GiB as the unit of measurement for pruning's sake. Right now it specifies it with a uint32 of bytes, which practically means that there are effectively 6 configurations that can come out of this: 1.5, 2.0, 2.5, 3.0, 3.5, and 4.0 GiB?

I'm interpreting this because you said we can't keep partial block files so that means that we effectively only have the discrete intervals of 512MiB to work with.

Comment on lines +1141 to +1148
if cfg.Prune != 0 && cfg.Prune < pruneMinSize {
err := fmt.Errorf("%s: the minimum value for --prune is %d. Got %d",
funcName, pruneMinSize, cfg.Prune)
fmt.Fprintln(os.Stderr, err)
fmt.Fprintln(os.Stderr, usageMessage)
return nil, nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to terminate here or should we just log a Warning and start up with the min value?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If it startups up, then the user may end up believing that their min value is correct. It's hard to catch the logs since there's a bunch at startup so I think it's better to fail.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems reasonable

Comment on lines +1149 to +1156
if cfg.Prune != 0 && cfg.TxIndex {
err := fmt.Errorf("%s: the --prune and --txindex options may "+
"not be activated at the same time", funcName)
fmt.Fprintln(os.Stderr, err)
fmt.Fprintln(os.Stderr, usageMessage)
return nil, nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are forcing this set of options to be blocked at the gate, shouldn't we return an error from PruneBlock on the txindex instead of returning nil as a noop?

Comment on lines +1157 to +1164
if cfg.Prune != 0 && cfg.AddrIndex {
err := fmt.Errorf("%s: the --prune and --addrindex options may "+
"not be activated at the same time", funcName)
fmt.Fprintln(os.Stderr, err)
fmt.Fprintln(os.Stderr, usageMessage)
return nil, nil, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are forcing this set of options to be blocked at the gate, shouldn't we return an error from PruneBlock on the addrindex instead of returning nil as a noop?

//
// This is part of the Indexer interface.
func (idx *TxIndex) PruneBlock(dbTx database.Tx, blockHash chainhash.Hash) error {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this ought to be an error given the rest of this PR

//
// This is part of the Indexer interface.
func (idx *AddrIndex) PruneBlock(dbTx database.Tx, blockHash chainhash.Hash) error {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like this ought to be an error given the rest of this PR

database/ffldb/blockio.go Outdated Show resolved Hide resolved
database/ffldb/blockio.go Outdated Show resolved Hide resolved
@kcalvinalvin kcalvinalvin force-pushed the add-pruning branch 2 times, most recently from 5227745 to 9a33437 Compare July 21, 2023 06:11
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Amazing work, I think the code looks good to go.

I tested several things on testnet and found no bugs:

  • Sync from empty state to chain tip without pruning
  • Reduce pruning to large value slightly below the actual size (30GB), expect only a few block files to be pruned
  • Reduce pruning to 20GB, expect more block files to be pruned
  • Reduce pruning to min value, expect all but three block files to be pruned
  • Remove pruning again, make sure the node continues working
  • Start new sync from empty state to chain tip with pruning set to minimal value, expect there never to be more than 3 block files to be present

I found one small UX thing that we might want to fix:
When running btcctl getblockchaininfo there's a boolean pruned that is always set to false, even when pruning is turned on. Should be fairly easy to add, so I'm waiting for that before fully approving the PR.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Nice work! I love how small the final diff actually is. I had prior versions that attempted to do more fine grained accounting, which weren't anywhere near as clean as this implementation is.

My main question in the diff is how we should handle the existing indexes already preseent:

  • Should we still allow txindex since it allows you to fetch blocks beyond the prune horizon?
  • Should we attempt to delete the old information for existing indexes, making them partially present, or just wipe the entire thing?
  • Should we continue to keep the compact filters as well, since they can be used to still serve light clients?

database/ffldb/blockio.go Outdated Show resolved Hide resolved
database/ffldb/blockio.go Outdated Show resolved Hide resolved
}

log.Tracef("Using %d more bytes than the target of %d MiB. Pruning files...",
totalSize-targetSize,
Copy link
Member

Choose a reason for hiding this comment

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

As is, doesn't this fail to account for the size of the last file on disk which may not be full? Or is the intended feature contract here that: we support pruning, but you can have N GB + maxBlockFile bytes on disk?

Copy link
Collaborator Author

@kcalvinalvin kcalvinalvin Aug 4, 2023

Choose a reason for hiding this comment

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

N GB + maxBlockFile is the current implementation.

I didn't give that much thought into it since you'll at max be 512MiB off. Which wouldn't really change the user experience as the utxo set is much bigger than that.

Since we're already fetching the lastFileSize, I could change it so that the last block file length is accounted for in the size limit.

EDIT: Ok the totalSize does account for the last file on the disk. The calculation for totalSize is lastFileSize + maxSize * numberOfAllFiles except for the last one which may not be full

database/ffldb/db.go Outdated Show resolved Hide resolved
database/ffldb/db.go Show resolved Hide resolved
blockchain/chain.go Outdated Show resolved Hide resolved
@@ -809,6 +809,16 @@ func (idx *AddrIndex) DisconnectBlock(dbTx database.Tx, block *btcutil.Block,
return nil
}

// PruneBlock is invoked when an older block is deleted after it's been
// processed.
// NOTE: For AddrIndex, it's a no-op as AddrIndex isn't allowed to be enabled
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't we want to reclaim the space on disk due to the addr index? Or is the idea that you need to first drop the entire thing, then enable pruning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Currently if you have had the addr index enabled, it won't prune the ones you've already indexed unless you do it manually.

I guess we could do either one of these:

1: Don't do anything with the already present addr index data (current)
2: Drop the addr index if pruning is enabled
3: Force the user to drop the addr index before allowing them to enable pruning

I'd favor either 1 or 3. Explicit user action or doing nothing seems better.

Copy link
Member

Choose a reason for hiding this comment

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

3: Force the user to drop the addr index before allowing them to enable pruning

I prefer this path myself. As otherwise, the queries of indexes like the addr index wouldn't be possible as it wants to return the block regions where the transaction exist, but they may not be accessible after pruning.

The only index that can still serve its purpose with pruned data is the neutrino filter index, as querying it doesn't require any block data at all (just slinging out the premade filters for each block).

@@ -255,6 +255,37 @@ func (idx *CfIndex) DisconnectBlock(dbTx database.Tx, block *btcutil.Block,
return nil
}

// PruneBlock is invoked when an older block is deleted after it's been
// processed. For cfindx, PruneBlock is exactly the same as disconnecting a block.
// TODO (kcalvinalvin): Consider keeping the filters at a later date to help with
Copy link
Member

Choose a reason for hiding this comment

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

Does btcwallet ask for the filters(not familiar with the codebase)? Might be worth keeping the filters around for it?

We don't do that yet but it's something we've been meaning on adding.

blockchain/indexers/cfindex.go Outdated Show resolved Hide resolved
server.go Show resolved Hide resolved
This change is part of the effort to add pruning support to btcd.

scanBlockFiles nows supports scanning files from an arbitrary block
number.  When blocks are pruned, the file number may not start from 0.
To account for this, scanBlockFiles now scans and retreives the start
and the end block file numbers and scans those files.
@kcalvinalvin
Copy link
Collaborator Author

  • Should we still allow txindex since it allows you to fetch blocks beyond the prune horizon?

I'm not sure. We could allow for it and just return empty strings when you attempt to fetch an already pruned block.

  • Should we attempt to delete the old information for existing indexes, making them partially present, or just wipe the entire thing?

Refer to #1971 (comment)

  • Should we continue to keep the compact filters as well, since they can be used to still serve light clients?

I'm indifferent either way. The user does have the option to drop the entire thing if they want. Though it may be nice to be able to only keep a portion of it?

Copy link
Collaborator

@halseth halseth left a comment

Choose a reason for hiding this comment

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

LGTM 🦩

@guggero guggero self-requested a review August 15, 2023 07:56
Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

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

When running btcctl getblockchaininfo there's a boolean pruned that is always set to false, even when pruning is turned on. Should be fairly easy to add, so I'm waiting for that before fully approving the PR.

I think this isn't addressed yet. Other than that (and the other remaining comments), this is good to go!

This change is part of the effort to add pruning support to btcd.

PruneBlocks will prune the earliest block files until it reaches the
given target size.  The returned hashes are the hashes of the blocks
that were pruned.
This change is part of the effort to add pruning support to btcd.

A field for pruning is added and the BlockChain struct is now able to be
configured with pruning.  Prune is called on every block connect and
the prune target field is passed to PruneBlocks func.  There's no check
to keep the latest 288 blocks to abide by the NODE_NETWORK_LIMITED rule.
That'll have to be enforced by the caller creating the BlockChain
struct.
This change is part of the effort to add pruning support to btcd.

Wire now supports the ability to signal NODE_NETWORK_LIMITED which
signals to peers that the node is able to serve the last 288 blocks.

Since archival nodes have all blocks, they can also signal for
NODE_NETWORK_LIMITED.  SFNodeNetworkLimited flag is added to the default
services.
@kcalvinalvin
Copy link
Collaborator Author

kcalvinalvin commented Aug 22, 2023

Made some additional changes:

1: Correctly return the prune status for getblockchaininfo
2: Got rid of pruning code for indexers. Compact filters are no longer pruned when the blocks are deleted. User either has all the cfindex data or none.
3: Force user to drop already existing addr and/or tx index when trying to enable pruning.
4: Error if user enables addr and/or tx index while pruned.
5: Error if user tries to enable cfindex if they started the ibd while pruned and cfindex disabled.
6: Force user to drop already indexed cfindex data when trying to disable cfindex while pruned.

@kcalvinalvin
Copy link
Collaborator Author

The last push just adds a more descriptive error message if the user tries to disable cfindex while the node is already pruned and has cfindex enabled.

This change is part of the effort to add pruning support to btcd.

Pruning is now available to the end user via --prune flag.  There are
checks in place so that the user doesn't go below the minimum prune
target of 1536 MiB.  The minimum is set so that we keep at least 288
blocks per the requirement for NODE_NETWORK_LIMITED nodes specified by
BIP0159.  The default value of 0 will disable pruning.
You can have a txindex but with the actual blocks gone, they won't be
much of a help.  Consider allowing these option to be both on in the
future where the txindex is only indexing the non-pruned blocks.
You can have a addrindex but with the actual blocks gone, they won't be
much of a help.  Consider allowing these option to be both on in the
future where the addrindex is only indexing the non-pruned blocks.
This change is part of the effort to add pruning support to btcd.

BeenPruned will return true if the database has ever been pruned.  This
allows for accurate prune status to be reported as well as ux
improvements by disallowing the user to accidently remove or enable
indexes.
This change is part of the effort to add pruning support to btcd.

Allowing the user to not pass in the --prune flag after pruning results
in inaccurate reporting of the prune status for getblockchaininfo and
for signaling NODE_NETWORK_LIMITED to peers.  Anything that relies on
cfg.Prune to be accurate is at risk of being incorrect.

To solve the current problems and to prevent potential future problems,
just force the user to keep the prune flag on like bitcoind.  In terms
of UX, there isn't that much of a loss since if the user wants to keep
more blocks than they previously did, they can just increase the size
passed to --prune.
This change is part of the effort to add pruning support to btcd.

Now that pruning is allowed in btcd, accurately report the prune status
back to the user.
This change is part of the effort to add pruning support to btcd.

The added *Initialized() functions to each of the indexers allow for
callers to check if each of the indexes have been created.  It's
useful for ux improvements where we force the user to manually drop
indexes that aren't compatible with pruning when the user enables
pruning.
This change is part of the effort to add pruning support to btcd.

The addr and tx indexes are not useful when the node is pruned as the
actual block data that the indexes point to are gone.  If the user had
previously enabled them, then explicitly require an action from the user
to remove the indexes before letting the user enable pruning.
This change is part of the effort to add pruning support to btcd.

It's not possible to generate the addr or tx indexes from scratch if the
block storage had been pruned previously as it's missing the block data.
When the user asks to create these indexes, tell them it's not possible
and the only way it's possible is if they delete and start anew.
This change is part of the effort to add pruning support to btcd.

cfIndex is a useful index even if the node has been pruned so it's
allowed to be enabled together with pruning.  However, if the user had
disabled cfindex and enabled pruning, it's not possible to generate
them.  In this case, we tell the user that it's impossible unless the
user deletes and start anew.

Additionally, if the user had enabled cfindex and also enabled pruning
from the start, don't let the user turn the cfindex off without dropping
it explicitly.  This is to make sure that the user isn't left at an
inconsistent state where the cfindex isn't able to catch up to the tip
because the blocks have already been pruned.
@kcalvinalvin
Copy link
Collaborator Author

Sorry for yet another additional push. 3 things here.

  1. The biggest thing: I realized that a lot of the code depends on cfg.Prune to tell if the node is pruned or not. But the code allowed the user to omit the flag even if the node had been previously pruned. This would've resulted in broadcasting to peers that the node isn't pruned and also would've given the wrong prune status for getblockchaininfo.

Instead of trying to see every case where I'd also have to call the database for the prune status, I added a commit that forces the user to prune if the database had been pruned previously like bitcoind. I don't think this really affects the UX and negates future bugs as well that can arise from not checking db for the prune status and just relying on cfg.Prune.

  1. getblockchaininfo now depends on cfg.Prune != 0
  2. Added additional message for the --prune flag on -h that tells the user about the default value of cfg.Prune == 0 and how that disables pruning.

@Roasbeef
Copy link
Member

This would've resulted in broadcasting to peers that the node isn't pruned

Can you elaborate on this? Broadcasting as in the set of services bits we advertise, or something else? IIUC, those service bits will always give away if a node has been pruned or not.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🎏

+1 to the additional safeguarding around user configuration/downgrades

@Roasbeef Roasbeef merged commit ec401d0 into btcsuite:master Aug 23, 2023
3 checks passed
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.

Pruning support?
8 participants