Ignore new blocks when -stopatheight target has been reached

[jonasschnelli]

If the -stopatheight target has been reached, a shutdown will be triggered, though, already loaded blocks will still be activated, making it impossible to precisely stop at a certain height.

This PR will ignore future blocks once the -stopatheight target has been reached.

Fixes #13477

[l2a5b1]

Can you explain the && ShutdownRequested() in the boolean expression?

If it is here to catch the StartShutdown() below, the do .. while loop is already terminated before it can reach this if statement.

        if (nStopAtHeight && pindexNewTip && pindexNewTip->nHeight >= nStopAtHeight) {
            LogPrintf("-stopatheight target (%d) reached, shutting down\n", nStopAtHeight);
            StartShutdown();
        }

...

        if (ShutdownRequested())
            break;
    } while (pindexNewTip != pindexMostWork);

[jonasschnelli]

The upper if statement with the ShutdownRequested() does make sure that on future calls of ActivateBestChain() and after StartShutdown() will not process blocks when stopheight is set at a shutdown is triggered.

The lower if statement is there to trigger the shutdown.

[NicolasDorier]

What I don't understand is that if ShutdownRequested() is false, then you will end up processing one more block, and this is not what you want. Removing ShutdownRequested() and just call StartShutdown at this place?

[fanquake]

Note previous discussion about -stopatheight in #13490.

[laanwj]

Note previous discussion about -stopatheight in #13490.

I tend to agree with @sdaftuar's reasoning that it is undesirable to burden the validation code further with this.

Then again, if we're not going to fix this we should close #13477 (with "wontfix" and this reasoning), otherwise people will wasting time trying to fix it.

[DrahtBot]

The last travis run for this pull request was 50 days ago and is thus outdated. To trigger a fresh travis build, this pull request should be closed and re-opened.

[Sjors]

This change is already a lot smaller than the previous attempt.

Being able to produce a deterministic UTXO set for a given height, as well as block files and indexes, could be quite useful. There may be other approaches than -stopatheight, or perhaps we can fix -stopatheight in a less disruptive way, e.g. by rolling back after overshooting.

[Sjors]

Lightly tested on testnet and it seems to indeed fix the issue.

[NicolasDorier]

@jonasschnelli this is not good.
Here is what I wished I could do:

bitcoind -stopatheight 500000
# Wait it reached it
bitcoin-cli gettxoutsetinfo
# Note the UTXOSet hash
bitcoin-cli stop
tar xcf "bitcoin-chainstate-snapshot-500000.tar" ~/.bitcoin/chainstate
# Upload the  bitcoin-chainstate-snapshot-500000.tar on a public location
# Sign the snapshot shasum

Then somebody who wants to verify and sign the snapshot as well would repeat

# On reference machine 
bitcoind -stopatheight 500000
# Wait it reached it
bitcoin-cli gettxoutsetinfo
# Note the UTXOSet hash

Then try to import the UTXO set from the tar

# On low powered device
tar xvf  bitcoin-chainstate-snapshot-500000.tar -C ~/.bitcoin/chainstate
bitcoind -stopatheight 500000
bitcoin-cli gettxoutsetinfo
# Check that the utxoset hash is the same as on the reference machine

Then he can add his signature to the shasum of bitcoin-chainstate-snapshot-500000.tar so people can trust it more.

If you stop the node once the height is reached, then there is no way to have the hash of the utxoset with gettxoutsetinfo, and no way for somebody else to independently verify a given utxoset inside a tar file.

[Sjors]

@NicolasDorier I totally agree your workflow is what we should strive for. However this PR only fixes a bug in the existing behaviour. Let's redefine -stopatheight (e.g. by adding a stop_sync_but_dont_exit param) in a followup PR.

[NicolasDorier]

Ah got it, this is already the current situation. I misunderstood thinking it was changing the behavior.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #15948 (refactor: rename chainActive 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.

[DrahtBot]

Needs rebase

[DrahtBot]

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.