[FEATURE] Implement stopatheight

[allocz]

Change Description

When the flag stopatheight is set to something greater than zero, a chain notification subscription is created, when the subscription handler receives a notification containing a connected block with height equal or greater than stopatheight value a server shutdown request is sent.

The current implementation is very simple, but further blocks may be processed after the stopheight, so the user may use the rpc to invalidate the block after the target height to get the expected utxoset.

This is a very useful feature for testing purposes.

Steps to Test

To test that the node is stopping when the expected height is reached, execute btcd with the flag --stopatheight=N, the node should stop after processing the block N, it can be verified by starting the node with --connect=127.0.0.1:invalid_port, the log will show that the chainstate is at height >= N.

Pull Request Checklist

Testing

• Your PR passes all CI checks.
• Tests covering the positive and negative (error paths) are included.
• Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

• The change is not insubstantial. Typo fixes are not accepted to fight bot spam.
• The change obeys the Code Documentation and Commenting guidelines, and lines wrap at 80.
• Commits follow the Ideal Git Commit Structure.
• Any new logging statements use an appropriate subsystem and logging level.

📝 Please see our Contribution Guidelines for further guidance.

[TechLateef]

This is a cool feature!

One minor observation: I noticed the PR doesn't reference any issue. It's usually best to link it to an issue so it can be tracked properly.
If an issue already exists, could you update the description to link it (something like Closes #IssueNumber)? If not, it might be worth opening a quick issue to anchor the feature proposal.

[allocz]

Opened the issue as suggested by @TechLateef.

This PR Closes #2535

[TechLateef]

sm.Stop() already handles graceful shutdown correctly it closes sm.quit to signal goroutines and calls sm.wg.Wait() to let them finish. The SIGTERM workaround entirely bypasses this cleanup process.
While the TODO suggests sm.Stop() causes a hang, the right fix is to investigate which specific goroutine is failing to respond to sm.quit, rather than signaling ourselves with an OS-level kill command.

[allocz]

Fixed this, now syncManager receives a done channel when started by peerHandler, when the stopHeight is reached, syncManager closes the done channel and exits, peerHandler sees that syncManager exited and requests the server shutdown.

[TechLateef]

LGTM! The updated approach is much cleaner. It would be good to add a test case to cover the stopatheight behaviour though.

[allocz]

Extracted stopatheight logic into the stopHeightReached function, to be able to test the logic in isolation. Also added tests.

[kcalvinalvin]

I see the usefulness of this for ibd related testing but 2 caveats.

1. The flag should be hidden for the regular user.
2. There's way too many changes here for the feature. This becomes a maintainability issue down the road.

For 1, the fix is pretty simple. If you add "hidden" to the config flag then it no longer prints when you give the help flag.
I'm ok with this.

+       StopAtHeight         int           `long:"stopatheight" hidden:"true" description:"Stop running after the block at the given height in the main chain is processed. Blocks after the target height may be processed during shutdown. For testing only (default: 0, disabled)"`

For 2, right now there's way too much going on for this 1 feature. We touch pretty critical parts of the code too and we're hurting readability for the functions which in turn, hurt maintainability. Because of this, I don't think the currently implemented code should be merged.

My preferred implementation would be something like this where we isolate the code for the feature like the code below. This wouldn't stop at the exact height but for ibd testing purposes, this is ok.

diff --git a/config.go b/config.go
index 06a8798b..5d260490 100644
--- a/config.go
+++ b/config.go
@@ -172,6 +172,7 @@ type config struct {
        SigNet               bool          `long:"signet" description:"Use the signet test network"`
        SigNetChallenge      string        `long:"signetchallenge" description:"Connect to a custom signet network defined by this challenge instead of using the global default signet test network -- Can be specified multiple times"`
        SigNetSeedNode       []string      `long:"signetseednode" description:"Specify a seed node for the signet network instead of using the global default signet network seed nodes"`
+       StopAtHeight         int           `long:"stopatheight" hidden:"true" description:"Stop running after the block at the given height in the main chain is processed. Blocks after the target height may be processed during shutdown. For testing only (default: 0, disabled)"`
        TestNet3             bool          `long:"testnet" description:"Use the test network (version 3)"`
        TestNet4             bool          `long:"testnet4" description:"Use the test network (version 4)"`
        TorIsolation         bool          `long:"torisolation" description:"Enable Tor stream isolation by randomizing user credentials for each connection."`
diff --git a/server.go b/server.go
index b94abdca..5c4165bd 100644
--- a/server.go
+++ b/server.go
@@ -2996,6 +2996,28 @@ func newServer(listenAddrs, agentBlacklist, agentWhitelist []string,
                return nil, err
        }

+       // stopatheight is a testing aid: once the main chain connects the
+       // configured height, request a graceful shutdown through the same
+       // path as an interrupt signal. The notification fires with the chain
+       // lock released, so blocking on the channel here is safe.
+       if stopHeight := int32(cfg.StopAtHeight); stopHeight > 0 {
+               var stopOnce sync.Once
+               s.chain.Subscribe(func(n *blockchain.Notification) {
+                       if n.Type != blockchain.NTBlockConnected {
+                               return
+                       }
+                       block, ok := n.Data.(*btcutil.Block)
+                       if !ok || block.Height() < stopHeight {
+                               return
+                       }
+                       stopOnce.Do(func() {
+                               srvrLog.Infof("Reached stopatheight %d; "+
+                                       "requesting shutdown", stopHeight)
+                               shutdownRequestChannel <- struct{}{}
+                       })
+               })
+       }
+
        // Search for a FeeEstimator state in the database. If none can be found
        // or if it cannot be loaded, create a new one.
        db.Update(func(tx database.Tx) error {

[allocz]

The nice thing about not allowing block processing after the stop height is that I can start the node in offline mode to inspect the utxo state, which will not be the desired state in the case of the node processing blocks after the stop height.

If the precision of stopping at the given height is not needed, this feature is also not needed, because we can just keep calling the block height via rpc and send the shutdown signal.

[kcalvinalvin]

The nice thing about not allowing block processing after the stop height is that I can start the node in offline mode to inspect the utxo state, which will not be the desired state in the case of the node processing blocks after the stop height.

If that's the end goal, you don't even need this because you can sync past the desired block and use invalidateblock rpc to reorg back to the desired chainstate.

If the precision of stopping at the given height is not needed, this feature is also not needed, because we can just keep calling the block height via rpc and send the shutdown signal.

This wasn't a stated goal in the original comment though...

[allocz]

If that's the end goal, you don't even need this because you can sync past the desired block and use invalidateblock rpc to reorg back to the desired chainstate.

Good point, now I agree with you.

[allocz]

@kcalvinalvin, I have addressed the requested changes, thanks for the suggestion.

[allocz]

The following is the diff of a test for this feature. It Requires #2571 to work.

diff --git a/integration/stopatheight_test.go b/integration/stopatheight_test.go
new file mode 100644
index 00000000..c5a406c5
--- /dev/null
+++ b/integration/stopatheight_test.go
@@ -0,0 +1,54 @@
+//go:build rpctest
+
+package integration_test
+
+import (
+       "testing"
+
+       "github.com/btcsuite/btcd/integration/rpctest"
+       "github.com/stretchr/testify/require"
+)
+
+func TestStopAtHeight(t *testing.T) {
+       // Generate the blocks.
+       h, err := rpctest.New()
+       require.NoError(t, err)
+       err = h.SetUp(rpctest.SOpts{
+               // Attempt to generate 1_000_000 mature utxos, which would take
+               // a long time, but stopheight will make we stop quickly.
+               UTXOCount: 1_000_000,
+               // The node may stop before wallet syncs up, disabling wallet
+               // synchronization wait avoids blocking forever.
+               NoWalletWait: true,
+               Args: []string{"--stopatheight=3"},
+       })
+       // The client may be unexpectedly disconnected when stopheight is
+       // reached, so we explicitly ignore the error here.
+       _ = err
+
+       // Block until node stops itself successfully after reaching the target
+       // height.
+       require.NoError(t, h.TearDown(rpctest.TOpts{
+               // Skip node cleanup, since we want to start the node again.
+               NoNodeCleanup: true,
+               NoShutdownSignal: true,
+       }))
+
+       // Start node again and assert we are at or after the expected height.
+       //
+       // Since the shutdown signal is sent upon receiving a block connection
+       // notification, we may stop in a height higher than the stopheight, for
+       // cases where we need the exact utxoset at certain height, we should
+       // manually invalidate further blocks.
+       //
+       // We don't start the wallet here because the wallet won't receive
+       // connected block events from the RPC, since the node already processed
+       // the blocks. The current wallet implementation does not handle
+       // fetching blocks from the node, but only processes connected block
+       // events from RPC notifications.
+       require.NoError(t, h.SetUp(rpctest.SOpts{NoWalletWait: true}))
+       blockCount, err := h.Client.GetBlockCount()
+       require.NoError(t, err)
+       require.Equal(t, true, blockCount >= 3)
+       require.NoError(t, h.TearDown())
+}