p2p, validation: Don't download witnesses for assumed-valid blocks when running in prune mode

dergoegge commented at 3:11 pm on February 6, 2023: member

I got a little nerd sniped by this idea, so I created this draft. I have done very little testing so far (syncing signet worked, currently syncing mainnet).

This PR does two things when running in prune mode: (a) assume witness merkle roots to be valid for assumed-valid blocks and (b) request assumed-valid blocks without MSG_WITNESS_FLAG.

In theory this is a good idea, because witnesses are not validated (i.e. they are assumed valid up to a certain block -assumevalid) and get pruned anyway when running in prune mode, so not downloading them (for assumed-valid blocks) reduces the bandwidth that is needed for IBD (don’t have any numbers on this yet).

One downside is that nodes serving blocks without witnesses can’t serve them directly from disk. They have to un-serialize and re-serialize without the witnesses before sending them.

Even though this is a draft, conceptual and approach review is very welcome.

TODO:

a bunch of tests
edge cases around manually pruned nodes (https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1419751142)
make sure we are not advertising blocks with witnesses when we in fact do not have the witnesses (https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1419443888, #27050 (comment))
edge case when restarting with different assume valid settings (https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1419751142)
document that it’s a change in what constitutes assumevalid (https://github.com/bitcoin/bitcoin/pull/27050#issuecomment-1419611954)
always serve pre-segwit blocks straight from disk (https://github.com/bitcoin/bitcoin/pull/27050#discussion_r1097532417)

DrahtBot commented at 3:11 pm on February 6, 2023: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	petertodd, ghost
Concept ACK	theStack, john-moffett, kristapsk, ajtowns, naumenkogs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
#26326 (net: don’t lock cs_main while reading blocks in net processing by andrewtoth)
#25781 (Remove almost all blockstorage globals by MarcoFalke)
#24008 (assumeutxo: net_processing changes by jamesob)
#15606 (assumeutxo by jamesob)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

in src/net_processing.cpp:2387 in 1abf5b7110 outdated

2380@@ -2380,6 +2381,15 @@ uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const
2381     return nFetchFlags;
2382 }
2383 
2384+uint32_t PeerManagerImpl::GetBlockFetchFlags(const Peer& peer, const CBlockIndex* index) const
2385+{
2386+    uint32_t fetch_flags = GetFetchFlags(peer);
2387+    if (m_chainman.m_blockman.IsPruneMode() && WITH_LOCK(m_chainman.GetMutex(), return m_chainman.IsAssumedValid(index))) {

andrewtoth commented at 3:23 pm on February 6, 2023:

Can we also check if we are after the first segwit block? The blocks before have no witness data so don’t require any more bandwidth, and then peers can still serve directly from disk.

dergoegge commented at 4:26 pm on February 6, 2023:

Yes I think that would be possible, although I wonder if that should happen on the server side (That is, have the server always serve pre-segwit blocks straight from disk). It doesn’t seem like it makes sense to support serving pre-segwit blocks not directly from disk.

theStack commented at 4:45 pm on February 6, 2023: contributor

Concept ACK

john-moffett commented at 4:53 pm on February 6, 2023: contributor

Concept ACK

Sjors commented at 5:18 pm on February 6, 2023: member

As @sipa pointed out in the Stack Overflow:

assumevalid currently in Bitcoin Core only skips script validation. But there are other rules that involve witness data, such as resource limits and the rule that no witness data is allowed for non-witness inputs.

So it shifts the meaning of (our baked in) -assumevalid somewhat, but only for pruned nodes. Uses may not expect that syncing with -prune performs less validation than without. So this probably needs a separate configuration flag.

One downside is that nodes serving blocks without witnesses can’t serve them directly from disk. They have to un-serialize and re-serialize without the witnesses before sending them.

Do you mean serving nodes that don’t have the witness data? Or do you mean nodes that do (but that’s already the case then)?

For nodes that come fresh out of IBD we should carefully test the behavior before and after the assumevalid drops below the prune height. Because at that point we should signal NODE_WITNESS. For better discoverability we should probably always announce that. In that case we should start downloading witness data a bit before the assumevalid block if we’re a recent release. But this gets tricky, because the node doesn’t know how big future blocks are, so doesn’t know when to start downloading witness data. We could change the release process to ensure assumevalid is deep enough to account for that.

sdaftuar commented at 5:26 pm on February 6, 2023: member

One potential issue with this is that when we write blocks to disk, we would be writing them without witness data. I think this could create a problem if you shut down a node in the middle of IBD and restart it with a different assumevalid value (such as disabling assumevalid) – we would think we have a block that is invalid, because we’re missing the witnesses.

While it’s an interesting thought experiment to see how much work would be required to optimize for this use case, I’m a bit skeptical that the complexity of implementing this optimization is worth it. But if you keep working on it and get to something that you think is robust I would be curious to see what it looks like.

luke-jr commented at 6:35 pm on February 6, 2023: member

I’m not sure skipping size checks is substantially different from skipping the script evaluation for security purposes.

But I also tend to agree with @sdaftuar that the complexity needed here probably isn’t worth it. Additionally, some thought would need to be given to privacy implications (we already identify version explicitly, but do we want to make full history validation an externally-visible attribute?).

sipa commented at 7:15 pm on February 6, 2023: member

One potential issue with this is that when we write blocks to disk, we would be writing them without witness data. I think this could create a problem if you shut down a node in the middle of IBD and restart it with a different assumevalid value (such as disabling assumevalid) – we would think we have a block that is invalid, because we’re missing the witnesses.

Good point; it may require some flag in the blockindex to note “valid modulo the possible the lack of witness data”.

While it’s an interesting thought experiment to see how much work would be required to optimize for this use case, I’m a bit skeptical that the complexity of implementing this optimization is worth it. But if you keep working on it and get to something that you think is robust I would be curious to see what it looks like.

It’s a pretty substantial savings in IBD bandwidth; in the last 100000 blocks, 43% of the serialized size is in witness data (47.8 GiB out of 110.6 GiB). Maybe that makes it worth looking into more?

I’m not sure skipping size checks is substantially different from skipping the script evaluation for security purposes.

Agree. I think it’s important to note that it’s a change in what constitutes assumevalid, but it’s not like it’s harder to run validation for the new set of things covered by it (in fact, it’s hard to just check script validity without checking the rest…).

Additionally, some thought would need to be given to privacy implications (we already identify version explicitly, but do we want to make full history validation an externally-visible attribute?).

The change only affects IBD, and only affects blocks that won’t be available to peers anymore once IBD is over (even in the scenario of interrupting and changing assumevalid in between). Are there other privacy concerns you’re worried about?

sdaftuar commented at 9:02 pm on February 6, 2023: member

It’s a pretty substantial savings in IBD bandwidth; in the last 100000 blocks, 43% of the serialized size is in witness data (47.8 GiB out of 110.6 GiB). Maybe that makes it worth looking into more?

Well I was more focused on my perception of the complexity than the benefit I guess, but you may be right. I have a number of thoughts that likely could be addressed if we can think about these issues clearly enough, but some things to consider:

whether we are in “prune mode” is a slightly nuanced idea. Nodes that are configured to have huge block databases and nodes that only prune when manually told to would both qualify as “pruning nodes” in the logic used here, which means that nodes might download blocks without witness and never actually delete them. (If we restarted with pruning disabled, then we would need to detect that our block storage is missing witnesses and force a reindex, which in turn would cause us to have to redownload all those blocks.). This is all doable logic, but just a messy edge case to consider.
we should be careful about having our code be robust to situations where a pruning node might find itself relaying a block at its tip even while it’s catching up, and if the user has set their assumevalid close to the network’s tip then you might imagine a problem being created from the pruning node advertising a block without the witness data to go with it. Maybe there is a clean way to think about this, where we enforce that a pruning node will never announce a block unless the block and the prior 288 blocks were all downloaded with witness?

When I first started thinking about this, I was contemplating that assumeutxo would be where we should spend our engineering resources, but now that I think a little more, maybe this feature (if we can design it properly) would compose nicely with assumeutxo (specifically around how much background validation happens when a node downloads the chain from scratch – maybe we could allow users to have different levels of validation like we do today).

luke-jr commented at 1:26 am on February 7, 2023: member

it may require some flag in the blockindex to note “valid modulo the possible the lack of witness data”.

Frankly, we should probably have a way to indicate this for every softfork anyway. And in that sense, it probably makes more sense as a per-rule-introduction range of blocks that have been checked, rather than a flag on each block index entry. But that may be complicated due to the existence of multiple conflicting chains.

(It might be possible to “cheat” on this issue for now and simply require those blocks to get pruned immediately.)

The change only affects IBD, and only affects blocks that won’t be available to peers anymore once IBD is over (even in the scenario of interrupting and changing assumevalid in between). Are there other privacy concerns you’re worried about?

My thought is that during IBD, your peers will know you’re doing an assumevalid IBD. This might actually rise to the level of a security issue since they then know they might be able to get away with feeding you blocks with invalid witnesses…

Sjors commented at 9:06 am on February 7, 2023: member

@luke-jr wrote:

might be able to get away with feeding you blocks with invalid witnesses

I don’t believe so, because up until you’ve synced headers beyond the assumevalid block, your node is indistinguishable. Once you have the headers, your node has the initiative and is going to ask specific blocks. (And you don’t need the witness data itself to check the block hash).

kristapsk commented at 2:36 pm on February 7, 2023: contributor

Concept ACK

dergoegge commented at 2:41 pm on February 7, 2023: member

@Sjors

Do you mean serving nodes that don’t have the witness data?

My comment was about serving nodes that store witnesses, because we do not support an archival full node mode that doesn’t keep the witnesses.

Or do you mean nodes that do (but that’s already the case then)?

Yes even currently, if asked for blocks without witnesses then we need to read, un-serialize and re-serialize without the witnesses. I was trying to imply that this does not happen very often currently, as (Bitcoin Core) nodes will always ask for blocks with witnesses from peers that signal NODE_WITNESS. However with this PR, pruned nodes will primarily ask for blocks without the witnesses during IBD (up to the assume-valid point), which results in some extra computational cost for serving nodes (this can be optimized a little bit, as andrewtoth pointed out.) The relevant logic can be found here.

Thank you all for the discussion so far! I updated the PR description to include the edge cases and suggestions mentioned here as TODOs.

petertodd commented at 1:22 am on February 9, 2023: contributor

It’s important to recognize that this is a security reduction, even though it only applies to assumed-valid blocks: one of the reasons why assume-valid is acceptable, is because the data that we’re assuming is valid is widely available for independent auditing and our peers can’t distinguish between nodes that are and are not actually checking it. Skipping the downloading of witnesses negates that security by making it possible to sync a pruned node without anyone having that data available.

A concrete example where this could become relevant is in the event that courts order Bitcoin node operators to not serve specific blocks that are claimed to contain illegal data: an adversary can easily spin up patched Bitcoin nodes that do not serve those witnesses, in conjunction with the distribution of a new assumed-valid hash.

Similarly, in conjunction with the above - or as a stand alone action - courts can simply order Bitcoin devs/node runners to use an assumed valid hash that contains a transaction with an invalid signature to confiscate/“recover” funds. This of course is a possible outcome of the Craig Wright case.

The difference with this patch from the status quo, is we’d already be shipping software that without any user intervention other than running with the appropriate assumed valid flag, would appear to work even though it was impossible for anyone to prove that something nefarious was going on.

NACK - This is not worth the consequences for a 43% savings.

Sjors commented at 11:57 am on February 9, 2023: member

an adversary can easily spin up patched Bitcoin nodes that do not serve those witnesses, in conjunction with the distribution of a new assumed-valid hash.

Not without also putting enough PoW in the chain with the alternative assumed-valid hash. This attack doesn’t seem cheaper than a regular 51% large reorg.

I do agree with these two more general concerns, but I’m not sure if they apply here, because the proposal is limited to pruned nodes:

1.

the data that we’re assuming is valid is widely available for independent auditing

Archival nodes will still have it. Pruned nodes only have it for recent blocks, just like they do for non-witness data.

2.

and our peers can’t distinguish between nodes that are and are not actually checking it.

This is partially the case, because nodes won’t reveal if they’re going to ask for the witness data until they have the headers. It also helps if during IBD we don’t reveal whether the node is going to be pruned or not (I assume we set NODE_NETWORK_LIMITED and NODE_NETWORK after IBD, but I haven’t checked). An attacker could still make an educated guess based on defaults and majority behavior, I don’t know if that’s exploitable.

petertodd commented at 12:59 pm on February 9, 2023: contributor

On February 9, 2023 12:57:27 PM GMT+01:00, Sjors Provoost @.***> wrote:

an adversary can easily spin up patched Bitcoin nodes that do not serve those witnesses, in conjunction with the distribution of a new assumed-valid hash.

Not without also putting enough PoW in the chain with the alternative assumed-valid hash. This attack doesn’t seem cheaper than a regular 51% large reorg.

The status quo does not trust miners. What you are suggesting does trust miners.

Recently the US based Foundry USA approached 50% of hash power, and it appears that much of the hashing power pointed to them is contractually obliged to mine at Foundry. Assuming that miners only mine valid blocks is not a good idea.

Another way of looking at it is if you aren’t validating signatures, at least indirectly, why are you even bothering to download witnesses? Why not just use a UTXO snapshot? You aren’t even doing a good job of inflation control: theft of lost coins is an example of inflation.

Sjors commented at 1:59 pm on February 9, 2023: member

To clarify, so the scenario you have in mind here is a prolonged >51% attack involving blocks with invalid witness data, where in addition they have coopted Bitcoin Core to ship a false assumevalid block?

No existing node would follow that chain. Nor would any unpruned node. The concern here is about freshly started pruned nodes.

in src/validation.cpp:3616 in 16a8a6bd9c outdated

3611@@ -3612,7 +3612,8 @@ static bool ContextualCheckBlock(const CBlock& block, BlockValidationState& stat
3612     //   {0xaa, 0x21, 0xa9, 0xed}, and the following 32 bytes are SHA256^2(witness root, witness reserved value). In case there are
3613     //   multiple, the last one is used.
3614     bool fHaveWitness = false;
3615-    if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT)) {
3616+    if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_SEGWIT) &&
3617+        (!chainman.m_blockman.IsPruneMode() || WITH_LOCK(chainman.GetMutex(), return !chainman.IsAssumedValid(block.GetHash())))) {

ajtowns commented at 3:40 am on February 10, 2023:

Perhaps clearer to write

0bool check_witness = DeploymentActiveAfter(..);
1if (check_witness && IsPruneMode()) {
2    LOCK(chainman.GetMutex());
3    if (IsAssumedValid()) check_witness = false;
4}
5if (check_witness) { ... }

ajtowns commented at 4:03 am on February 10, 2023: contributor

Concept ACK; saving a substantial amount of download bandwidth for data we’re not going to verify and will discard anyway seems worthwhile.

I agree with luke’s comment on recording whether we downloaded (and validated) witness data and on how to do that. (Recording (rule-name, hash, since-height) might work – that would create an extra entry for each rule for each valid-fork entry in getchaintips, I think, which seems okay)

I think it would probably make sense to have a separate option, say -prepruneassumevalidwitness (softset to on when -prune is above 550 and assumevalid is set), so that people can turn off this behaviour without sacrificing ibd performance, for when they’re doing weird special cases (particularly if they’re running a recently released version with an assumevalid block that’s only a couple of months old).

JoseSK999 commented at 3:59 pm on February 10, 2023: none

Skipping the downloading of witnesses negates that security by making it possible to sync a pruned node without anyone having that data available.

It’s true that, if witness data from Assumevalid blocks is not available, the new type of pruned node will be depositing more trust on a previously reviewed Assumevalid (the specific release of Assumevalid cannot be audited now because the witnesses are missing). So the trust assumption here is: If there is no archival node accessible, we still trust an Assumevalid release that was audited/reviewed although it’s not possible to currently audit it.

This is true because archival nodes, by definition, store all witness data. So if there’s no archival node but, for some reason there are nodes serving all non-witness data and witness data from non-Assumevalid blocks, this type of pruned node will operate under a stronger assumption (that a previous Assumevalid release is valid, although not auditable).

However Bitcoin is already operating under the assumption that archival nodes are available. If they weren’t available no full node would be able to sync. If the new pruned node type is introduced, these nodes could still sync if only witness data from Assumevalid blocks is missing.

Ruling out a systematic failure that makes all archival nodes to lose only witness data from Assumevalid blocks, the only way something like that could happen is if all archival nodes decided to selectively delete/not serve Assumevalid witness data, but store/serve everything else. If no node served historic data in general (witness and non-witness), it would be impossible for our new type of pruned node to sync, and if all “archival” nodes were pre-segwit, then our node couldn’t sync either.

It seems to me that this doesn’t change anything fundamentally. I could of course be wrong.

dergoegge commented at 4:23 pm on February 10, 2023: member

@petertodd Thanks for commenting and challenging the approach!

If I understand correctly, your main concern is about witness availability. As it currently stands that availability is required to sync a full node (pruned or not), as we are checking that the witness merkle root that is committed to matches with the witnesses that are actually downloaded. So even though we are skipping script validation, we require the exact witnesses (which we are assuming to be valid) to be available. By enabling pruned nodes to not download the witnesses (which requires skipping the witness merkle root check), we remove the requirement of witness availability (at least for assumed-valid blocks) to sync a pruned node. Witness availability is obviously important to be able to audit the chain with -assumevalid=0, which is a big part of our security assumptions behind assume-valid.

You are not arguing that stopping to request witnesses (i.e. removing the need to have them available for syncing) somehow makes them less available, right? My thoughts on this would be similar/the same to what Sjors said: pruned nodes already don’t serve historical witnesses and this PR does not prevent archival nodes from storing/serving them, so IMO witness availability is not directly threatened by this PR.

iiuc, you are saying that removing the witness availability requirement makes it possible to assume a chain valid without ever seeing the invalid witnesses (since witnesses are no longer forced to be revealed). This seems entirely possible but updating the default assume valid hash in Bitcoin Core always involves auditing (I would expect anyone to audit their own choice of that hash as well) and a chain that fails the audit due to witness unavailability would not be made the default. Of course like you say, a court could try to mandate the inclusion of a specific assume-valid hash but that just leads us to the final backstop we always have: As long as a economic majority can’t be forced to upgrade their nodes, the new rules won’t be adopted. Under the assumption that that is in fact possible, a court can mandate worse things than an invalid assume-valid hash.

It is currently not possible to set the assume-valid hash to a chain with unavailable witnesses, however it is currently possible to set it to a chain with available but invalid witnesses. What is worse, unavailable or invalid witnesses? I would argue, that they are equally bad because they can achieve the exact same nefarious outcomes.

The difference with this patch from the status quo, is we’d already be shipping software that without any user intervention other than running with the appropriate assumed valid flag, would appear to work even though it was impossible for anyone to prove that something nefarious was going on.

Being forced to assume a chain valid which I can’t audit (due to witness unavailability), sounds like a clear indication of something nefarious going on to me (but yea, one couldn’t prove that).

Sjors commented at 9:38 am on February 11, 2023: member

One thing we could do to mitigate the concern of missing witness data, is to download it for a small random sample of blocks. When that fails, we fall back to redownloading all witness data (which would presumably also fail). So in a scenario where witness data is really gone, pruned nodes will behave the same as unpruned nodes during IBD.

naumenkogs commented at 10:32 am on February 13, 2023: member

Concept ACK. I think the 43% saving is a big deal, and the code change amount seems fine, although worth prototyping those corner cases first :)

W.r.t. the security reduction pointed out by @petertodd, I agree with the @JoseSK999 comment almost entirely. It could be emphasized further — 43% saving by itself helps the node-running culture as a whole (more than it hurts it by dropping witness checks).

I agree with @sdaftuar it plays nicely together with assumeutxo.

petertodd commented at 1:47 pm on February 13, 2023: contributor

@dergoegge

I would expect anyone to audit their own choice of that hash as well

Notice how in the assume-UTXO work, we’ve chosen to not even give users the ability to choose their own assumed-valid UTXO set. Instead, the hashes are fixed by Bitcoin Core because we don’t trust users to audit their own choice of UTXO set. @naumenkogs assumeutxo is much more likely to “help node running culture” in terms of getting more pruned nodes up and running. And it has the advantage of having much more clear security considerations: you are trusting the assumed UTXO.

Anyway, a way to mitigate these concerns about the reduction in security margin would be to simply put “skip-witness” mode behind a command line flag and leave it default off. Remember that many, probably most, nodes have ample bandwidth available and thus won’t actually see an improvement from skipping witness downloads. That’s certainly been the case for most of my nodes, which are mostly in data centers, and have been IO/CPU limited during IBD, not bandwidth limited. “Node-in-a-box” solutions like Umbrel and Start9, more frequently used on slower home internet connections and Tor, can consider enabling the skip-witness mode by default (or perhaps after a bandwidth test).

Sjors commented at 12:49 pm on February 14, 2023: member

Making witness skipping opt-in makes sense. In addition our GUI initial setup wizard could help the user with this (as it does with suggesting pruning).

wtogami commented at 7:21 am on March 1, 2023: contributor

Could we please pick a different name for old witnessless syncing than “pruned”? It’s different from the existing pruned node type that entirely discards old blocks. Old witnessless nodes could reindex and rescan as well as provide old witnessless blocks to other nodes doing IBD of this type. We need a new name for this.

dergoegge commented at 10:28 am on March 1, 2023: member

@wtogami This PR does not introduce a new witnessless archival mode (which is what you are describing iiuc). It only makes pruned nodes skip downloading the witnesses up to the assume-valid point (since they’re not validated and deleted shortly after anyway).

I would agree that witnessless archival nodes should not be called “pruned”, but again that is not what this PR is introducing.

dergoegge force-pushed on Mar 9, 2023

ghost commented at 6:14 pm on March 9, 2023: none

It’s important to recognize that this is a security reduction, even though it only applies to assumed-valid blocks: one of the reasons why assume-valid is acceptable, is because the data that we’re assuming is valid is widely available for independent auditing and our peers can’t distinguish between nodes that are and are not actually checking it. Skipping the downloading of witnesses negates that security by making it possible to sync a pruned node without anyone having that data available.

A concrete example where this could become relevant is in the event that courts order Bitcoin node operators to not serve specific blocks that are claimed to contain illegal data: an adversary can easily spin up patched Bitcoin nodes that do not serve those witnesses, in conjunction with the distribution of a new assumed-valid hash.

Similarly, in conjunction with the above - or as a stand alone action - courts can simply order Bitcoin devs/node runners to use an assumed valid hash that contains a transaction with an invalid signature to confiscate/“recover” funds. This of course is a possible outcome of the Craig Wright case.

The difference with this patch from the status quo, is we’d already be shipping software that without any user intervention other than running with the appropriate assumed valid flag, would appear to work even though it was impossible for anyone to prove that something nefarious was going on.

NACK - This is not worth the consequences for a 43% savings.

The status quo does not trust miners. What you are suggesting does trust miners.

Recently the US based Foundry USA approached 50% of hash power, and it appears that much of the hashing power pointed to them is contractually obliged to mine at Foundry. Assuming that miners only mine valid blocks is not a good idea.

Another way of looking at it is if you aren’t validating signatures, at least indirectly, why are you even bothering to download witnesses? Why not just use a UTXO snapshot? You aren’t even doing a good job of inflation control: theft of lost coins is an example of inflation.

Love the adversarial thinking shared in quoted comments by Peter Todd leaving aside the emotions.

NACK

I do not agree with the changes made in this pull request

Anyway, a way to mitigate these concerns about the reduction in security margin would be to simply put “skip-witness” mode behind a command line flag and leave it default off. Remember that many, probably most, nodes have ample bandwidth available and thus won’t actually see an improvement from skipping witness downloads. That’s certainly been the case for most of my nodes, which are mostly in data centers, and have been IO/CPU limited during IBD, not bandwidth limited. “Node-in-a-box” solutions like Umbrel and Start9, more frequently used on slower home internet connections and Tor, can consider enabling the skip-witness mode by default (or perhaps after a bandwidth test).

Maybe we can have such useless options similar to the patch created by Luke Dashjr recently

Note: Default matters a lot in bitcoin core and its used by 99% nodes. Nobody wants to play with their money and use different values unless they know what they are doing.

[validation] Extract assumed-valid logic 513879770c

[validation] Introduce BlockStatus::BLOCK_PRUNED_WITNESSES

The BlockStatus::BLOCK_PRUNED_WITNESSES flag is used to indicate when a
post-segwit block was stored on disk without its witnesses.

6ee5216138

[txdb] Add flag for pruned witnesses c61fecaded

[blockstorage] Add flag for pruned witnesses 6bb640d328

[init] Disallow restart in non-prune mode after blocks have been stored without witnesses

It can happen that no blocks were actually pruned while starting up in
prune mode (e.g. manual prune mode with `-prune=1`) and when restarting
with `-prune=0` we have to redownload if we stored blocks without their
witnesses.

92f6ba0d86

[validation] Assume merkle witness root valid for assumed-valid blocks in prune mode

This commit widens what we assume to be valid as part of our
assume-valid settings for *pruned* full nodes. Previously only script
checking was skipped (i.e they were assumed valid), but with this commit
the witness merkle root is no longer validated and therefore assumed
valid as well.

282b58ab33

[net processing] Don't announce/relay blocks for which we didn't store witnesses in prune mode

We want to avoid announcing and relaying blocks for which we do not have
the witnesses. Otherwise a peer might request the block expecting it to
include witnesses and disconnect us for failing to deliver.

TODO With the default max tip age of 24h it is *very* unlikely that we
will announce assumed-valid (i.e. witnessless) blocks while in prune
mode. So this commit is a bit of a belt and supenders, or maybe not even
necessary at all?

2ddd16c323

[net processing] Log inv command string for requested blocks cd59ddadb1

[net processing] Don't download witnesses for assumed-valid blocks in prune mode

If we are in prune mode, we can skip downloading witnesses for
assumed-valid blocks, as we will not be using the witnesses and
pruning/deleting them shortly after anyway.

7aafe8ab51

[test] Add feature_assumevalid_pruning.py 5d91fae234

dergoegge force-pushed on Mar 14, 2023

DrahtBot added the label Needs rebase on Mar 15, 2023

DrahtBot commented at 11:48 pm on March 15, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

in src/node/blockstorage.cpp:367 in 5d91fae234

363@@ -364,6 +364,12 @@ bool BlockManager::LoadBlockIndexDB(const Consensus::Params& consensus_params)
364         LogPrintf("LoadBlockIndexDB(): Block files have previously been pruned\n");
365     }
366 
367+    // Check whether we have ever pruned block & undo files

paulkania commented at 9:54 pm on March 18, 2023:

not sure what this comment means. “ever” could be a misspelt “every”. If that’s the case, best to change it to from ’every’ to ‘all the’

dergoegge commented at 2:03 pm on August 7, 2023: member

Can be marked up for grabs

dergoegge closed this on Aug 7, 2023

fanquake added the label Up for grabs on Aug 7, 2023

AndySchroder commented at 2:53 am on August 1, 2024: none

What about adding a mode that skips downloading witness data for assumed-valid blocks and also discards downloaded witness data after validating it (for non assumed-valid blocks), but doesn’t prune non-witness data at all?

p2p, validation: Don’t download witnesses for assumed-valid blocks when running in prune mode #27050

Reviews

Conflicts