diff --git a/.github/workflows/linkchecker.yml b/.github/workflows/linkchecker.yml deleted file mode 100644 index e2ba80861..000000000 --- a/.github/workflows/linkchecker.yml +++ /dev/null @@ -1,12 +0,0 @@ -name: Check Markdown links -on: - schedule: - - cron: '* */24 * * *' -jobs: - markdown-link-check: - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v3 - - uses: creachadair/github-action-markdown-link-check@master - with: - folder-path: "docs" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index ec4fa810b..2e0cd548c 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -21,7 +21,7 @@ jobs: go-version: '1.17' - name: Build - uses: goreleaser/goreleaser-action@v2 + uses: goreleaser/goreleaser-action@v3 if: ${{ github.event_name == 'pull_request' }} with: version: latest @@ -30,7 +30,7 @@ jobs: - run: echo https://github.com/tendermint/tendermint/blob/${GITHUB_REF#refs/tags/}/CHANGELOG.md#${GITHUB_REF#refs/tags/} > ../release_notes.md - name: Release - uses: goreleaser/goreleaser-action@v2 + uses: goreleaser/goreleaser-action@v3 if: startsWith(github.ref, 'refs/tags/') with: version: latest diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 65ab5ee3b..d38caf50b 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -21,6 +21,7 @@ Special thanks to external contributors on this release: - [rpc] \#7982 Add new Events interface and deprecate Subscribe. (@creachadair) - [cli] \#8081 make the reset command safe to use by intoducing `reset-state` command. Fixed by \#8259. (@marbar3778, @cmwaters) - [config] \#8222 default indexer configuration to null. (@creachadair) + - [rpc] \#8570 rework timeouts to be per-method instead of global. (@creachadair) - Apps diff --git a/abci/client/socket_client.go b/abci/client/socket_client.go index aa4fdcbe9..7dfcf76cc 100644 --- a/abci/client/socket_client.go +++ b/abci/client/socket_client.go @@ -17,12 +17,6 @@ import ( "github.com/tendermint/tendermint/libs/service" ) -const ( - // reqQueueSize is the max number of queued async requests. - // (memory: 256MB max assuming 1MB transactions) - reqQueueSize = 256 -) - // This is goroutine-safe, but users should beware that the application in // general is not meant to be interfaced with concurrent callers. type socketClient struct { @@ -48,7 +42,7 @@ var _ Client = (*socketClient)(nil) func NewSocketClient(logger log.Logger, addr string, mustConnect bool) Client { cli := &socketClient{ logger: logger, - reqQueue: make(chan *requestAndResponse, reqQueueSize), + reqQueue: make(chan *requestAndResponse), mustConnect: mustConnect, addr: addr, reqSent: list.New(), @@ -118,6 +112,11 @@ func (cli *socketClient) sendRequestsRoutine(ctx context.Context, conn io.Writer case <-ctx.Done(): return case reqres := <-cli.reqQueue: + // N.B. We must enqueue before sending out the request, otherwise the + // server may reply before we do it, and the receiver will fail for an + // unsolicited reply. + cli.trackRequest(reqres) + if err := types.WriteMessage(reqres.Request, bw); err != nil { cli.stopForError(fmt.Errorf("write to buffer: %w", err)) return @@ -158,14 +157,15 @@ func (cli *socketClient) recvResponseRoutine(ctx context.Context, conn io.Reader } } -func (cli *socketClient) willSendReq(reqres *requestAndResponse) { - cli.mtx.Lock() - defer cli.mtx.Unlock() - +func (cli *socketClient) trackRequest(reqres *requestAndResponse) { + // N.B. We must NOT hold the client state lock while checking this, or we + // may deadlock with shutdown. if !cli.IsRunning() { return } + cli.mtx.Lock() + defer cli.mtx.Unlock() cli.reqSent.PushBack(reqres) } @@ -199,7 +199,6 @@ func (cli *socketClient) doRequest(ctx context.Context, req *types.Request) (*ty } reqres := makeReqRes(req) - cli.willSendReq(reqres) select { case cli.reqQueue <- reqres: diff --git a/cmd/tendermint/commands/light.go b/cmd/tendermint/commands/light.go index 8e39d7900..2b812fe18 100644 --- a/cmd/tendermint/commands/light.go +++ b/cmd/tendermint/commands/light.go @@ -171,7 +171,8 @@ for applications built w/ Cosmos SDK). // If necessary adjust global WriteTimeout to ensure it's greater than // TimeoutBroadcastTxCommit. // See https://github.com/tendermint/tendermint/issues/3435 - if cfg.WriteTimeout <= conf.RPC.TimeoutBroadcastTxCommit { + // Note we don't need to adjust anything if the timeout is already unlimited. + if cfg.WriteTimeout > 0 && cfg.WriteTimeout <= conf.RPC.TimeoutBroadcastTxCommit { cfg.WriteTimeout = conf.RPC.TimeoutBroadcastTxCommit + 1*time.Second } diff --git a/config/config.go b/config/config.go index 5769c458b..fa9bf252e 100644 --- a/config/config.go +++ b/config/config.go @@ -523,7 +523,7 @@ func DefaultRPCConfig() *RPCConfig { MaxSubscriptionClients: 100, MaxSubscriptionsPerClient: 5, ExperimentalDisableWebsocket: false, // compatible with TM v0.35 and earlier - EventLogWindowSize: 0, // disables /events RPC by default + EventLogWindowSize: 30 * time.Second, EventLogMaxItems: 0, TimeoutBroadcastTxCommit: 10 * time.Second, diff --git a/docs/rfc/README.md b/docs/rfc/README.md index d944e72e7..2872c988a 100644 --- a/docs/rfc/README.md +++ b/docs/rfc/README.md @@ -57,5 +57,6 @@ sections. - [RFC-017: ABCI++ Vote Extension Propagation](./rfc-017-abci++-vote-extension-propag.md) - [RFC-018: BLS Signature Aggregation Exploration](./rfc-018-bls-agg-exploration.md) - [RFC-019: Configuration File Versioning](./rfc-019-config-version.md) +- [RFC-020: Onboarding Projects](./rfc-020-onboarding-projects.rst) diff --git a/docs/rfc/rfc-020-onboarding-projects.rst b/docs/rfc/rfc-020-onboarding-projects.rst new file mode 100644 index 000000000..dc18de65d --- /dev/null +++ b/docs/rfc/rfc-020-onboarding-projects.rst @@ -0,0 +1,240 @@ +======================================= +RFC 020: Tendermint Onboarding Projects +======================================= + +.. contents:: + :backlinks: none + +Changelog +--------- + +- 2022-03-30: Initial draft. (@tychoish) +- 2022-04-25: Imported document to tendermint repository. (@tychoish) + +Overview +-------- + +This document describes a collection of projects that might be good for new +engineers joining the Tendermint Core team. These projects mostly describe +features that we'd be very excited to see land in the code base, but that are +intentionally outside of the critical path of a release on the roadmap, and +have the following properties that we think make good on-boarding projects: + +- require relatively little context for the project or its history beyond a + more isolated area of the code. + +- provide exposure to different areas of the codebase, so new team members + will have reason to explore the code base, build relationships with people + on the team, and gain experience with more than one area of the system. + +- be of moderate size, striking a healthy balance between trivial or + mechanical changes (which provide little insight) and large intractable + changes that require deeper insight than is available during onboarding to + address well. A good size project should have natural touchpoints or + check-ins. + +Projects +-------- + +Before diving into one of these projects, have a conversation about the +project or aspects of Tendermint that you're excited to work on with your +onboarding buddy. This will help make sure that these issues are still +relevant, help you get any context, underatnding known pitfalls, and to +confirm a high level approach or design (if relevant.) On-boarding buddies +should be prepared to do some design work before someone joins the team. + +The descriptions that follow provide some basic background and attempt to +describe the user stories and the potential impact of these project. + +E2E Test Systems +~~~~~~~~~~~~~~~~ + +Tendermint's E2E framework makes it possible to run small test networks with +different Tendermint configurations, and make sure that the system works. The +tests run Tendermint in a separate binary, and the system provides some very +high level protection against making changes that could break Tendermint in +otherwise difficult to detect ways. + +Working on the E2E system is a good place to get introduced to the Tendermint +codebase, particularly for developers who are newer to Go, as the E2E +system (generator, runner, etc.) is distinct from the rest of Tendermint and +comparatively quite small, so it may be easier to begin making changes in this +area. At the same time, because the E2E system exercises *all* of Tendermint, +work in this area is a good way to get introduced to various components of the +system. + +Configurable E2E Workloads +++++++++++++++++++++++++++ + +All E2E tests use the same workload (e.g. generated transactions, submitted to +different nodes in the network,) which has been tuned empirically to provide a +gentle but consistent parallel load that all E2E tests can pass. Ideally, the +workload generator could be configurable to have different shapes of work +(bursty, different transaction sizes, weighted to different nodes, etc.) and +even perhaps further parameterized within a basic shape, which would make it +possible to use our existing test infrastructure to answer different questions +about the performance or capability of the system. + +The work would involve adding a new parameter to the E2E test manifest, and +creating an option (e.g. "legacy") for the current load generation model, +extract configurations options for the current load generation, and then +prototype implementations of alternate load generation, and also run some +preliminary using the tools. + +Byzantine E2E Workloads ++++++++++++++++++++++++ + +There are two main kinds of integration tests in Tendermint: the E2E test +framework, and then a collection of integration tests that masquerade as +unit-tests. While some of this expansion of test scope is (potentially) +inevitable, the masquerading unit tests (e.g ``consensus.byzantine_test.go``) +end up being difficult to understand, difficult to maintain, and unreliable. + +One solution to this, would be to modify the E2E ABCI application to allow it +to inject byzantine behavior, and then have this be a configurable aspect of +a test network to be able to provoke Byzantine behavior in a "real" system and +then observe that evidence is constructed. This would make it possible to +remove the legacy tests entirely once the new tests have proven themselves. + +Abstract Orchestration Framework +++++++++++++++++++++++++++++++++ + +The orchestration of e2e test processes is presently done using docker +compose, which works well, but has proven a bit limiting as all processes need +to run on a single machine, and the log aggregation functions are confusing at +best. + +This project would replace the current orchestration with something more +generic, potentially maintaining the current system, but also allowing the e2e +tests to manage processes using k8s. There are a few "local" k8s frameworks +(e.g. kind and k3s,) which might be able to be useful for our current testing +model, but hopefully, we could use this new implementation with other k8s +systems for more flexible distribute test orchestration. + +Improve Operationalize Experience of ``run-multiple.sh`` +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ + +The e2e test runner currently runs a single test, and in most cases we manage +the test cases using a shell script that ensure cleanup of entire test +suites. This is a bit difficult to maintain and makes reproduction of test +cases more awkward than it should be. The e2e ``runner`` itself should provide +equivalent functionality to ``run-multiple.sh``: ensure cleanup of test cases, +collect and process output, and be able to manage entire suites of cases. + +It might also be useful to implement an e2e test orchestrator that runs all +tendermint instances in a single process, using "real" networks for faster +feedback and iteration during development. + +In addition to being a bit easier to maintain, having a more capable runner +implementation would make it easier to collect data from test runs, improve +debugability and reporting. + +Fan-Out For CI E2E Tests +++++++++++++++++++++++++ + +While there are some parallelism in the execution of e2e tests, each e2e test +job must build a tendermint e2e image, which takes about 5 minutes of CPU time +per-task, which given the size of each of the runs. + +We'd like to be able to reduce the amount of overhead per-e2e tests while +keeping the cycle time for working with the tests very low, while also +maintaining a reasonable level of test coverage. This is an impossible +tradeoff, in some ways, and the percentage of overhead at the moment is large +enough that we can make some material progress with a moderate amount of time. + +Most of this work has to do with modifying github actions configuration and +e2e artifact (docker) building to reduce redundant work. Eventually, when we +can drop the requirement for CGo storage engines, it will be possible to move +(cross) compile tendermint locally, and then inject the binary into the docker +container, which would reduce a lot of the build-time complexity, although we +can move more in this direction or have runtime flags to disable CGo +dependencies for local development. + +Remove Panics +~~~~~~~~~~~~~ + +There are lots of places in the code base which can panic, and would not be +particularly well handled. While in some cases, panics are the right answer, +in many cases the panics were just added to simplify downstream error +checking, and could easily be converted to errors. + +The `Don't Panic RFC +`_ +covers some of the background and approach. + +While the changes are in this project are relatively rote, this will provide +exposure to lots of different areas of the codebase as well as insight into +how different areas of the codebase interact with eachother, as well as +experience with the test suites and infrastructure. + +Implement more Expressive ABCI Applications +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +Tendermint maintains two very simple ABCI applications (a KV application used +for basic testing, and slightly more advanced test application used in the +end-to-end tests). Writing an application would provide a new engineer with +useful experiences using Tendermint that mirrors the expierence of downstream +users. + +This is more of an exploratory project, but could include providing common +interfaces on top of Tendermint consensus for other well known protocols or +tools (e.g. ``etcd``) or a DNS server or some other tool. + +Self-Regulating Reactors +~~~~~~~~~~~~~~~~~~~~~~~~ + +Currently reactors (the internal processes that are responsible for the higher +level behavior of Tendermint) can be started and stopped, but have no +provision for being paused. These additional semantics may allow Tendermint to +pause reactors (and avoid processing their messhages, etc.) and allow better +coordination in the future. + +While this is a big project, it's possible to break this apart into many +smaller projects: make p2p channels pauseable, add pause/UN-pause hooks to the +service implementation and machinery, and finally to modify the reactor +implementations to take advantage of these additional semantics + +This project would give an engineer some exposure to the p2p layer of the +code, as well as to various aspects of the reactor implementations. + +Metrics +~~~~~~~ + +Tendermint has a metrics system that is relatively underutilized, and figuring +out ways to capture and organize the metrics to provide value to users might +provide an interesting set of projects for new engineers on Tendermint. + +Convert Logs to Metrics ++++++++++++++++++++++++ + +Because the tendermint logs tend to be quite verbose and not particularly +actionable, most users largely ignore the logging or run at very low +verbosity. While the log statements in the code do describe useful events, +taken as a whole the system is not particularly tractable, and particularly at +the Debug level, not useful. One solution to this problem is to identify log +messages that might be (e.g. increment a counter for certian kinds of errors) + +One approach might be to look at various logging statements, particularly +debug statements or errors that are logged but not returned, and see if +they're convertable to counters or other metrics. + +Expose Metrics to Tests ++++++++++++++++++++++++ + +The existing Tendermint test suites replace the metrics infrastructure with +no-op implementations, which means that tests can neither verify that metrics +are ever recorded, nor can tests use metrics to observe events in the +system. Writing an implementation, for testing, that makes it possible to +record metrics and provides an API for introspecting this data, as well as +potentially writing tests that take advantage of this type, could be useful. + +Logging Metrics ++++++++++++++++ + +In some systems, the logging system itself can provide some interesting +insights for operators: having metrics that track the number of messages at +different levels as well as the total number of messages, can act as a canary +for the system as a whole. + +This should be achievable by adding an interceptor layer within the logging +package itself that can add metrics to the existing system. diff --git a/go.mod b/go.mod index d359e7c1e..ae4929dc7 100644 --- a/go.mod +++ b/go.mod @@ -16,7 +16,7 @@ require ( github.com/gorilla/websocket v1.5.0 github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 - github.com/lib/pq v1.10.5 + github.com/lib/pq v1.10.6 github.com/libp2p/go-buffer-pool v0.0.2 github.com/mroth/weightedrand v0.4.1 github.com/oasisprotocol/curve25519-voi v0.0.0-20210609091139-0a56a4bca00b diff --git a/go.sum b/go.sum index d53c7f74e..c05aab943 100644 --- a/go.sum +++ b/go.sum @@ -821,8 +821,8 @@ github.com/lib/pq v1.0.0/go.mod h1:5WUZQaWbwv1U+lTReE5YruASi9Al49XbQIvNi/34Woo= github.com/lib/pq v1.8.0/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= github.com/lib/pq v1.9.0/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= github.com/lib/pq v1.10.4/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= -github.com/lib/pq v1.10.5 h1:J+gdV2cUmX7ZqL2B0lFcW0m+egaHC2V3lpO8nWxyYiQ= -github.com/lib/pq v1.10.5/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= +github.com/lib/pq v1.10.6 h1:jbk+ZieJ0D7EVGJYpL9QTz7/YW6UHbmdnZWYyK5cdBs= +github.com/lib/pq v1.10.6/go.mod h1:AlVN5x4E4T544tWzH6hKfbfQvm3HdbOxrmggDNAPY9o= github.com/libp2p/go-addr-util v0.1.0/go.mod h1:6I3ZYuFr2O/9D+SoyM0zEw0EF3YkldtTX406BpdQMqw= github.com/libp2p/go-buffer-pool v0.0.1/go.mod h1:xtyIz9PMobb13WaxR6Zo1Pd1zXJKYg0a8KiIvDp3TzQ= github.com/libp2p/go-buffer-pool v0.0.2 h1:QNK2iAFa8gjAe1SPz6mHSMuCcjs+X1wlHzeOSqcmlfs= diff --git a/internal/blocksync/pool.go b/internal/blocksync/pool.go index 30bb6962e..64ce54dc6 100644 --- a/internal/blocksync/pool.go +++ b/internal/blocksync/pool.go @@ -280,7 +280,7 @@ func (pool *BlockPool) AddBlock(peerID types.NodeID, block *types.Block, extComm pool.mtx.Lock() defer pool.mtx.Unlock() - if block.Height != extCommit.Height { + if extCommit != nil && block.Height != extCommit.Height { return fmt.Errorf("heights don't match, not adding block (block height: %d, commit height: %d)", block.Height, extCommit.Height) } @@ -597,7 +597,9 @@ func (bpr *bpRequester) setBlock(block *types.Block, extCommit *types.ExtendedCo return false } bpr.block = block - bpr.extCommit = extCommit + if extCommit != nil { + bpr.extCommit = extCommit + } bpr.mtx.Unlock() select { diff --git a/internal/blocksync/reactor.go b/internal/blocksync/reactor.go index 4ba6d28a0..c1b032b03 100644 --- a/internal/blocksync/reactor.go +++ b/internal/blocksync/reactor.go @@ -185,31 +185,39 @@ func (r *Reactor) OnStop() { // Otherwise, we'll respond saying we do not have it. func (r *Reactor) respondToPeer(ctx context.Context, msg *bcproto.BlockRequest, peerID types.NodeID, blockSyncCh p2p.Channel) error { block := r.store.LoadBlock(msg.Height) - if block != nil { - extCommit := r.store.LoadBlockExtendedCommit(msg.Height) - if extCommit == nil { - return fmt.Errorf("found block in store without extended commit: %v", block) - } - blockProto, err := block.ToProto() - if err != nil { - return fmt.Errorf("failed to convert block to protobuf: %w", err) - } - + if block == nil { + r.logger.Info("peer requesting a block we do not have", "peer", peerID, "height", msg.Height) return blockSyncCh.Send(ctx, p2p.Envelope{ - To: peerID, - Message: &bcproto.BlockResponse{ - Block: blockProto, - ExtCommit: extCommit.ToProto(), - }, + To: peerID, + Message: &bcproto.NoBlockResponse{Height: msg.Height}, }) } - r.logger.Info("peer requesting a block we do not have", "peer", peerID, "height", msg.Height) + state, err := r.stateStore.Load() + if err != nil { + return fmt.Errorf("loading state: %w", err) + } + var extCommit *types.ExtendedCommit + if state.ConsensusParams.ABCI.VoteExtensionsEnabled(msg.Height) { + extCommit = r.store.LoadBlockExtendedCommit(msg.Height) + if extCommit == nil { + return fmt.Errorf("found block in store with no extended commit: %v", block) + } + } + + blockProto, err := block.ToProto() + if err != nil { + return fmt.Errorf("failed to convert block to protobuf: %w", err) + } return blockSyncCh.Send(ctx, p2p.Envelope{ - To: peerID, - Message: &bcproto.NoBlockResponse{Height: msg.Height}, + To: peerID, + Message: &bcproto.BlockResponse{ + Block: blockProto, + ExtCommit: extCommit.ToProto(), + }, }) + } // handleMessage handles an Envelope sent from a peer on a specific p2p Channel. @@ -242,12 +250,16 @@ func (r *Reactor) handleMessage(ctx context.Context, envelope *p2p.Envelope, blo "err", err) return err } - extCommit, err := types.ExtendedCommitFromProto(msg.ExtCommit) - if err != nil { - r.logger.Error("failed to convert extended commit from proto", - "peer", envelope.From, - "err", err) - return err + var extCommit *types.ExtendedCommit + if msg.ExtCommit != nil { + var err error + extCommit, err = types.ExtendedCommitFromProto(msg.ExtCommit) + if err != nil { + r.logger.Error("failed to convert extended commit from proto", + "peer", envelope.From, + "err", err) + return err + } } if err := r.pool.AddBlock(envelope.From, block, extCommit, block.Size()); err != nil { @@ -440,6 +452,8 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh lastRate = 0.0 didProcessCh = make(chan struct{}, 1) + + initialCommitHasExtensions = (r.initialState.LastBlockHeight > 0 && r.store.LoadBlockExtendedCommit(r.initialState.LastBlockHeight) != nil) ) defer trySyncTicker.Stop() @@ -463,12 +477,27 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh ) switch { - // TODO(sergio) Might be needed for implementing the upgrading solution. Remove after that - //case state.LastBlockHeight > 0 && r.store.LoadBlockExtCommit(state.LastBlockHeight) == nil: - case state.LastBlockHeight > 0 && blocksSynced == 0: - // Having state-synced, we need to blocksync at least one block + + // The case statement below is a bit confusing, so here is a breakdown + // of its logic and purpose: + // + // If VoteExtensions are enabled we cannot switch to consensus without + // the vote extension data for the previous height, i.e. state.LastBlockHeight. + // + // If extensions were required during state.LastBlockHeight and we have + // sync'd at least one block, then we are guaranteed to have extensions. + // BlockSync requires that the blocks it fetches have extensions if + // extensions were enabled during the height. + // + // If extensions were required during state.LastBlockHeight and we have + // not sync'd any blocks, then we can only transition to Consensus + // if we already had extensions for the initial height. + // If any of these conditions is not met, we continue the loop, looking + // for extensions. + case state.ConsensusParams.ABCI.VoteExtensionsEnabled(state.LastBlockHeight) && + (blocksSynced == 0 && !initialCommitHasExtensions): r.logger.Info( - "no seen commit yet", + "no extended commit yet", "height", height, "last_block_height", state.LastBlockHeight, "initial_height", state.InitialHeight, @@ -520,18 +549,19 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh // see if there are any blocks to sync first, second, extCommit := r.pool.PeekTwoBlocks() - if first == nil || second == nil || extCommit == nil { - if first != nil && extCommit == nil { - // See https://github.com/tendermint/tendermint/pull/8433#discussion_r866790631 - panic(fmt.Errorf("peeked first block without extended commit at height %d - possible node store corruption", first.Height)) - } - // we need all to sync the first block + if first != nil && extCommit == nil && + state.ConsensusParams.ABCI.VoteExtensionsEnabled(first.Height) { + // See https://github.com/tendermint/tendermint/pull/8433#discussion_r866790631 + panic(fmt.Errorf("peeked first block without extended commit at height %d - possible node store corruption", first.Height)) + } else if first == nil || second == nil { + // we need to have fetched two consecutive blocks in order to + // perform blocksync verification continue - } else { - // try again quickly next loop - didProcessCh <- struct{}{} } + // try again quickly next loop + didProcessCh <- struct{}{} + firstParts, err := first.MakePartSet(types.BlockPartSizeBytes) if err != nil { r.logger.Error("failed to make ", @@ -557,7 +587,10 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh // validate the block before we persist it err = r.blockExec.ValidateBlock(ctx, state, first) } - + if err == nil && state.ConsensusParams.ABCI.VoteExtensionsEnabled(first.Height) { + // if vote extensions were required at this height, ensure they exist. + err = extCommit.EnsureExtensions() + } // If either of the checks failed we log the error and request for a new block // at that height if err != nil { @@ -593,7 +626,15 @@ func (r *Reactor) poolRoutine(ctx context.Context, stateSynced bool, blockSyncCh r.pool.PopRequest() // TODO: batch saves so we do not persist to disk every block - r.store.SaveBlock(first, firstParts, extCommit) + if state.ConsensusParams.ABCI.VoteExtensionsEnabled(first.Height) { + r.store.SaveBlockWithExtendedCommit(first, firstParts, extCommit) + } else { + // We use LastCommit here instead of extCommit. extCommit is not + // guaranteed to be populated by the peer if extensions are not enabled. + // Currently, the peer should provide an extCommit even if the vote extension data are absent + // but this may change so using second.LastCommit is safer. + r.store.SaveBlock(first, firstParts, second.LastCommit) + } // TODO: Same thing for app - but we would need a way to get the hash // without persisting the state. diff --git a/internal/blocksync/reactor_test.go b/internal/blocksync/reactor_test.go index 654289ed4..8ca12c4c1 100644 --- a/internal/blocksync/reactor_test.go +++ b/internal/blocksync/reactor_test.go @@ -40,8 +40,6 @@ type reactorTestSuite struct { blockSyncChannels map[types.NodeID]p2p.Channel peerChans map[types.NodeID]chan p2p.PeerUpdate peerUpdates map[types.NodeID]*p2p.PeerUpdates - - blockSync bool } func setup( @@ -69,7 +67,6 @@ func setup( blockSyncChannels: make(map[types.NodeID]p2p.Channel, numNodes), peerChans: make(map[types.NodeID]chan p2p.PeerUpdate, numNodes), peerUpdates: make(map[types.NodeID]*p2p.PeerUpdates, numNodes), - blockSync: true, } chDesc := &p2p.ChannelDescriptor{ID: BlockSyncChannel, MessageType: new(bcproto.Message)} @@ -97,21 +94,19 @@ func setup( return rts } -func (rts *reactorTestSuite) addNode( +func makeReactor( ctx context.Context, t *testing.T, nodeID types.NodeID, genDoc *types.GenesisDoc, privVal types.PrivValidator, - maxBlockHeight int64, -) { - t.Helper() + channelCreator p2p.ChannelCreator, + peerEvents p2p.PeerEventSubscriber) *Reactor { logger := log.NewNopLogger() - rts.nodes = append(rts.nodes, nodeID) - rts.app[nodeID] = proxy.New(abciclient.NewLocalClient(logger, &abci.BaseApplication{}), logger, proxy.NopMetrics()) - require.NoError(t, rts.app[nodeID].Start(ctx)) + app := proxy.New(abciclient.NewLocalClient(logger, &abci.BaseApplication{}), logger, proxy.NopMetrics()) + require.NoError(t, app.Start(ctx)) blockDB := dbm.NewMemDB() stateDB := dbm.NewMemDB() @@ -139,7 +134,7 @@ func (rts *reactorTestSuite) addNode( blockExec := sm.NewBlockExecutor( stateStore, log.NewNopLogger(), - rts.app[nodeID], + app, mp, sm.EmptyEvidencePool{}, blockStore, @@ -147,44 +142,35 @@ func (rts *reactorTestSuite) addNode( sm.NopMetrics(), ) - var lastExtCommit *types.ExtendedCommit + return NewReactor( + logger, + stateStore, + blockExec, + blockStore, + nil, + channelCreator, + peerEvents, + true, + consensus.NopMetrics(), + nil, // eventbus, can be nil + ) +} - // The commit we are building for the current height. - seenExtCommit := &types.ExtendedCommit{} +func (rts *reactorTestSuite) addNode( + ctx context.Context, + t *testing.T, + nodeID types.NodeID, + genDoc *types.GenesisDoc, + privVal types.PrivValidator, + maxBlockHeight int64, +) { + t.Helper() - for blockHeight := int64(1); blockHeight <= maxBlockHeight; blockHeight++ { - lastExtCommit = seenExtCommit.Clone() + logger := log.NewNopLogger() - thisBlock := sf.MakeBlock(state, blockHeight, lastExtCommit.StripExtensions()) - thisParts, err := thisBlock.MakePartSet(types.BlockPartSizeBytes) - require.NoError(t, err) - blockID := types.BlockID{Hash: thisBlock.Hash(), PartSetHeader: thisParts.Header()} - - // Simulate a commit for the current height - vote, err := factory.MakeVote( - ctx, - privVal, - thisBlock.Header.ChainID, - 0, - thisBlock.Header.Height, - 0, - 2, - blockID, - time.Now(), - ) - require.NoError(t, err) - seenExtCommit = &types.ExtendedCommit{ - Height: vote.Height, - Round: vote.Round, - BlockID: blockID, - ExtendedSignatures: []types.ExtendedCommitSig{vote.ExtendedCommitSig()}, - } - - state, err = blockExec.ApplyBlock(ctx, state, blockID, thisBlock) - require.NoError(t, err) - - blockStore.SaveBlock(thisBlock, thisParts, seenExtCommit) - } + rts.nodes = append(rts.nodes, nodeID) + rts.app[nodeID] = proxy.New(abciclient.NewLocalClient(logger, &abci.BaseApplication{}), logger, proxy.NopMetrics()) + require.NoError(t, rts.app[nodeID].Start(ctx)) rts.peerChans[nodeID] = make(chan p2p.PeerUpdate) rts.peerUpdates[nodeID] = p2p.NewPeerUpdates(rts.peerChans[nodeID], 1) @@ -193,21 +179,64 @@ func (rts *reactorTestSuite) addNode( chCreator := func(ctx context.Context, chdesc *p2p.ChannelDescriptor) (p2p.Channel, error) { return rts.blockSyncChannels[nodeID], nil } - rts.reactors[nodeID] = NewReactor( - rts.logger.With("nodeID", nodeID), - stateStore, - blockExec, - blockStore, - nil, - chCreator, - func(ctx context.Context) *p2p.PeerUpdates { return rts.peerUpdates[nodeID] }, - rts.blockSync, - consensus.NopMetrics(), - nil, // eventbus, can be nil - ) - require.NoError(t, rts.reactors[nodeID].Start(ctx)) - require.True(t, rts.reactors[nodeID].IsRunning()) + peerEvents := func(ctx context.Context) *p2p.PeerUpdates { return rts.peerUpdates[nodeID] } + reactor := makeReactor(ctx, t, nodeID, genDoc, privVal, chCreator, peerEvents) + + lastExtCommit := &types.ExtendedCommit{} + + state, err := reactor.stateStore.Load() + require.NoError(t, err) + for blockHeight := int64(1); blockHeight <= maxBlockHeight; blockHeight++ { + block, blockID, partSet, seenExtCommit := makeNextBlock(ctx, t, state, privVal, blockHeight, lastExtCommit) + + state, err = reactor.blockExec.ApplyBlock(ctx, state, blockID, block) + require.NoError(t, err) + + reactor.store.SaveBlockWithExtendedCommit(block, partSet, seenExtCommit) + lastExtCommit = seenExtCommit + } + + rts.reactors[nodeID] = reactor + require.NoError(t, reactor.Start(ctx)) + require.True(t, reactor.IsRunning()) +} + +func makeNextBlock(ctx context.Context, + t *testing.T, + state sm.State, + signer types.PrivValidator, + height int64, + lc *types.ExtendedCommit) (*types.Block, types.BlockID, *types.PartSet, *types.ExtendedCommit) { + + lastExtCommit := lc.Clone() + + block := sf.MakeBlock(state, height, lastExtCommit.ToCommit()) + partSet, err := block.MakePartSet(types.BlockPartSizeBytes) + require.NoError(t, err) + blockID := types.BlockID{Hash: block.Hash(), PartSetHeader: partSet.Header()} + + // Simulate a commit for the current height + vote, err := factory.MakeVote( + ctx, + signer, + block.Header.ChainID, + 0, + block.Header.Height, + 0, + 2, + blockID, + time.Now(), + ) + require.NoError(t, err) + seenExtCommit := &types.ExtendedCommit{ + Height: vote.Height, + Round: vote.Round, + BlockID: blockID, + ExtendedSignatures: []types.ExtendedCommitSig{vote.ExtendedCommitSig()}, + } + return block, blockID, partSet, seenExtCommit + } func (rts *reactorTestSuite) start(ctx context.Context, t *testing.T) { @@ -415,3 +444,35 @@ func TestReactor_BadBlockStopsPeer(t *testing.T) { len(rts.reactors[newNode.NodeID].pool.peers), ) } + +/* +func TestReactorReceivesNoExtendedCommit(t *testing.T) { + blockDB := dbm.NewMemDB() + stateDB := dbm.NewMemDB() + stateStore := sm.NewStore(stateDB) + blockStore := store.NewBlockStore(blockDB) + blockExec := sm.NewBlockExecutor( + stateStore, + log.NewNopLogger(), + rts.app[nodeID], + mp, + sm.EmptyEvidencePool{}, + blockStore, + eventbus, + sm.NopMetrics(), + ) + NewReactor( + log.NewNopLogger(), + stateStore, + blockExec, + blockStore, + nil, + chCreator, + func(ctx context.Context) *p2p.PeerUpdates { return rts.peerUpdates[nodeID] }, + rts.blockSync, + consensus.NopMetrics(), + nil, // eventbus, can be nil + ) + +} +*/ diff --git a/internal/consensus/common_test.go b/internal/consensus/common_test.go index 1dc92b33c..27fc39d6b 100644 --- a/internal/consensus/common_test.go +++ b/internal/consensus/common_test.go @@ -527,10 +527,11 @@ func loadPrivValidator(t *testing.T, cfg *config.Config) *privval.FilePV { } type makeStateArgs struct { - config *config.Config - logger log.Logger - validators int - application abci.Application + config *config.Config + consensusParams *types.ConsensusParams + logger log.Logger + validators int + application abci.Application } func makeState(ctx context.Context, t *testing.T, args makeStateArgs) (*State, []*validatorStub) { @@ -551,9 +552,13 @@ func makeState(ctx context.Context, t *testing.T, args makeStateArgs) (*State, [ if args.logger == nil { args.logger = log.NewNopLogger() } + c := factory.ConsensusParams() + if args.consensusParams != nil { + c = args.consensusParams + } state, privVals := makeGenesisState(ctx, t, args.config, genesisStateArgs{ - Params: factory.ConsensusParams(), + Params: c, Validators: validators, }) diff --git a/internal/consensus/msgs.go b/internal/consensus/msgs.go index c59c06a41..1024c24ae 100644 --- a/internal/consensus/msgs.go +++ b/internal/consensus/msgs.go @@ -222,11 +222,7 @@ func (*VoteMessage) TypeTag() string { return "tendermint/Vote" } // ValidateBasic checks whether the vote within the message is well-formed. func (m *VoteMessage) ValidateBasic() error { - // Here we validate votes with vote extensions, since we require vote - // extensions to be sent in precommit messages during consensus. Prevote - // messages should never have vote extensions, and this is also validated - // here. - return m.Vote.ValidateWithExtension() + return m.Vote.ValidateBasic() } // String returns a string representation. diff --git a/internal/consensus/reactor.go b/internal/consensus/reactor.go index 0ca28ec46..547763b6f 100644 --- a/internal/consensus/reactor.go +++ b/internal/consensus/reactor.go @@ -216,6 +216,8 @@ func (r *Reactor) OnStart(ctx context.Context) error { if err := r.state.Start(ctx); err != nil { return err } + } else if err := r.state.updateStateFromStore(); err != nil { + return err } go r.updateRoundStateRoutine(ctx) @@ -796,13 +798,20 @@ func (r *Reactor) gossipVotesRoutine(ctx context.Context, ps *PeerState, voteCh if blockStoreBase > 0 && prs.Height != 0 && rs.Height >= prs.Height+2 && prs.Height >= blockStoreBase { // Load the block's extended commit for prs.Height, which contains precommit // signatures for prs.Height. - if ec := r.state.blockStore.LoadBlockExtendedCommit(prs.Height); ec != nil { - if ok, err := r.pickSendVote(ctx, ps, ec, voteCh); err != nil { - return - } else if ok { - logger.Debug("picked Catchup commit to send", "height", prs.Height) - continue - } + var ec *types.ExtendedCommit + if r.state.state.ConsensusParams.ABCI.VoteExtensionsEnabled(prs.Height) { + ec = r.state.blockStore.LoadBlockExtendedCommit(prs.Height) + } else { + ec = r.state.blockStore.LoadBlockCommit(prs.Height).WrappedExtendedCommit() + } + if ec == nil { + continue + } + if ok, err := r.pickSendVote(ctx, ps, ec, voteCh); err != nil { + return + } else if ok { + logger.Debug("picked Catchup commit to send", "height", prs.Height) + continue } } diff --git a/internal/consensus/reactor_test.go b/internal/consensus/reactor_test.go index 0c49a3737..d848f53e7 100644 --- a/internal/consensus/reactor_test.go +++ b/internal/consensus/reactor_test.go @@ -32,6 +32,7 @@ import ( "github.com/tendermint/tendermint/internal/test/factory" "github.com/tendermint/tendermint/libs/log" tmcons "github.com/tendermint/tendermint/proto/tendermint/consensus" + tmproto "github.com/tendermint/tendermint/proto/tendermint/types" "github.com/tendermint/tendermint/types" ) @@ -600,6 +601,118 @@ func TestReactorCreatesBlockWhenEmptyBlocksFalse(t *testing.T) { wg.Wait() } +// TestSwitchToConsensusVoteExtensions tests that the SwitchToConsensus correctly +// checks for vote extension data when required. +func TestSwitchToConsensusVoteExtensions(t *testing.T) { + for _, testCase := range []struct { + name string + storedHeight int64 + initialRequiredHeight int64 + includeExtensions bool + shouldPanic bool + }{ + { + name: "no vote extensions but not required", + initialRequiredHeight: 0, + storedHeight: 2, + includeExtensions: false, + shouldPanic: false, + }, + { + name: "no vote extensions but required this height", + initialRequiredHeight: 2, + storedHeight: 2, + includeExtensions: false, + shouldPanic: true, + }, + { + name: "no vote extensions and required in future", + initialRequiredHeight: 3, + storedHeight: 2, + includeExtensions: false, + shouldPanic: false, + }, + { + name: "no vote extensions and required previous height", + initialRequiredHeight: 1, + storedHeight: 2, + includeExtensions: false, + shouldPanic: true, + }, + { + name: "vote extensions and required previous height", + initialRequiredHeight: 1, + storedHeight: 2, + includeExtensions: true, + shouldPanic: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + ctx, cancel := context.WithTimeout(context.Background(), time.Second*15) + defer cancel() + cs, vs := makeState(ctx, t, makeStateArgs{validators: 1}) + validator := vs[0] + validator.Height = testCase.storedHeight + + cs.state.LastBlockHeight = testCase.storedHeight + cs.state.LastValidators = cs.state.Validators.Copy() + cs.state.ConsensusParams.ABCI.VoteExtensionsEnableHeight = testCase.initialRequiredHeight + + propBlock, err := cs.createProposalBlock(ctx) + require.NoError(t, err) + + // Consensus is preparing to do the next height after the stored height. + cs.Height = testCase.storedHeight + 1 + propBlock.Height = testCase.storedHeight + blockParts, err := propBlock.MakePartSet(types.BlockPartSizeBytes) + require.NoError(t, err) + + var voteSet *types.VoteSet + if testCase.includeExtensions { + voteSet = types.NewExtendedVoteSet(cs.state.ChainID, testCase.storedHeight, 0, tmproto.PrecommitType, cs.state.Validators) + } else { + voteSet = types.NewVoteSet(cs.state.ChainID, testCase.storedHeight, 0, tmproto.PrecommitType, cs.state.Validators) + } + signedVote := signVote(ctx, t, validator, tmproto.PrecommitType, cs.state.ChainID, types.BlockID{ + Hash: propBlock.Hash(), + PartSetHeader: blockParts.Header(), + }) + + if !testCase.includeExtensions { + signedVote.Extension = nil + signedVote.ExtensionSignature = nil + } + + added, err := voteSet.AddVote(signedVote) + require.NoError(t, err) + require.True(t, added) + + if testCase.includeExtensions { + cs.blockStore.SaveBlockWithExtendedCommit(propBlock, blockParts, voteSet.MakeExtendedCommit()) + } else { + cs.blockStore.SaveBlock(propBlock, blockParts, voteSet.MakeExtendedCommit().ToCommit()) + } + reactor := NewReactor( + log.NewNopLogger(), + cs, + nil, + nil, + cs.eventBus, + true, + NopMetrics(), + ) + + if testCase.shouldPanic { + assert.Panics(t, func() { + reactor.SwitchToConsensus(ctx, cs.state, false) + }) + } else { + reactor.SwitchToConsensus(ctx, cs.state, false) + } + }) + } +} + func TestReactorRecordsVotesAndBlockParts(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), time.Minute) defer cancel() diff --git a/internal/consensus/replay_test.go b/internal/consensus/replay_test.go index c8f04655b..99d3c17a1 100644 --- a/internal/consensus/replay_test.go +++ b/internal/consensus/replay_test.go @@ -1204,14 +1204,16 @@ func (bs *mockBlockStore) LoadBlockMeta(height int64) *types.BlockMeta { } } func (bs *mockBlockStore) LoadBlockPart(height int64, index int) *types.Part { return nil } -func (bs *mockBlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.ExtendedCommit) { +func (bs *mockBlockStore) SaveBlockWithExtendedCommit(block *types.Block, blockParts *types.PartSet, seenCommit *types.ExtendedCommit) { +} +func (bs *mockBlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.Commit) { } func (bs *mockBlockStore) LoadBlockCommit(height int64) *types.Commit { - return bs.extCommits[height-1].StripExtensions() + return bs.extCommits[height-1].ToCommit() } func (bs *mockBlockStore) LoadSeenCommit() *types.Commit { - return bs.extCommits[len(bs.extCommits)-1].StripExtensions() + return bs.extCommits[len(bs.extCommits)-1].ToCommit() } func (bs *mockBlockStore) LoadBlockExtendedCommit(height int64) *types.ExtendedCommit { return bs.extCommits[height-1] diff --git a/internal/consensus/state.go b/internal/consensus/state.go index b016e2687..3af775bb6 100644 --- a/internal/consensus/state.go +++ b/internal/consensus/state.go @@ -122,9 +122,8 @@ type State struct { // store blocks and commits blockStore sm.BlockStore - stateStore sm.Store - initialStatePopulated bool - skipBootstrapping bool + stateStore sm.Store + skipBootstrapping bool // create and execute blocks blockExec *sm.BlockExecutor @@ -248,9 +247,6 @@ func NewState( } func (cs *State) updateStateFromStore() error { - if cs.initialStatePopulated { - return nil - } state, err := cs.stateStore.Load() if err != nil { return fmt.Errorf("loading state: %w", err) @@ -259,6 +255,15 @@ func (cs *State) updateStateFromStore() error { return nil } + eq, err := state.Equals(cs.state) + if err != nil { + return fmt.Errorf("comparing state: %w", err) + } + // if the new state is equivalent to the old state, we should not trigger a state update. + if eq { + return nil + } + // We have no votes, so reconstruct LastCommit from SeenCommit. if state.LastBlockHeight > 0 { cs.reconstructLastCommit(state) @@ -266,7 +271,6 @@ func (cs *State) updateStateFromStore() error { cs.updateToState(state) - cs.initialStatePopulated = true return nil } @@ -692,23 +696,54 @@ func (cs *State) sendInternalMessage(ctx context.Context, mi msgInfo) { } } -// Reconstruct LastCommit from SeenCommit, which we saved along with the block, -// (which happens even before saving the state) +// Reconstruct the LastCommit from either SeenCommit or the ExtendedCommit. SeenCommit +// and ExtendedCommit are saved along with the block. If VoteExtensions are required +// the method will panic on an absent ExtendedCommit or an ExtendedCommit without +// extension data. func (cs *State) reconstructLastCommit(state sm.State) { - extCommit := cs.blockStore.LoadBlockExtendedCommit(state.LastBlockHeight) - if extCommit == nil { - panic(fmt.Sprintf( - "failed to reconstruct last commit; commit for height %v not found", - state.LastBlockHeight, - )) + extensionsEnabled := cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(state.LastBlockHeight) + if !extensionsEnabled { + votes, err := cs.votesFromSeenCommit(state) + if err != nil { + panic(fmt.Sprintf("failed to reconstruct last commit; %s", err)) + } + cs.LastCommit = votes + return } - lastPrecommits := extCommit.ToVoteSet(state.ChainID, state.LastValidators) - if !lastPrecommits.HasTwoThirdsMajority() { - panic("failed to reconstruct last commit; does not have +2/3 maj") + votes, err := cs.votesFromExtendedCommit(state) + if err != nil { + panic(fmt.Sprintf("failed to reconstruct last extended commit; %s", err)) + } + cs.LastCommit = votes +} + +func (cs *State) votesFromExtendedCommit(state sm.State) (*types.VoteSet, error) { + ec := cs.blockStore.LoadBlockExtendedCommit(state.LastBlockHeight) + if ec == nil { + return nil, fmt.Errorf("extended commit for height %v not found", state.LastBlockHeight) + } + vs := ec.ToExtendedVoteSet(state.ChainID, state.LastValidators) + if !vs.HasTwoThirdsMajority() { + return nil, errors.New("extended commit does not have +2/3 majority") + } + return vs, nil +} + +func (cs *State) votesFromSeenCommit(state sm.State) (*types.VoteSet, error) { + commit := cs.blockStore.LoadSeenCommit() + if commit == nil || commit.Height != state.LastBlockHeight { + commit = cs.blockStore.LoadBlockCommit(state.LastBlockHeight) + } + if commit == nil { + return nil, fmt.Errorf("commit for height %v not found", state.LastBlockHeight) } - cs.LastCommit = lastPrecommits + vs := commit.ToVoteSet(state.ChainID, state.LastValidators) + if !vs.HasTwoThirdsMajority() { + return nil, errors.New("commit does not have +2/3 majority") + } + return vs, nil } // Updates State and increments height to match that of state. @@ -810,7 +845,11 @@ func (cs *State) updateToState(state sm.State) { cs.ValidRound = -1 cs.ValidBlock = nil cs.ValidBlockParts = nil - cs.Votes = cstypes.NewHeightVoteSet(state.ChainID, height, validators) + if state.ConsensusParams.ABCI.VoteExtensionsEnabled(height) { + cs.Votes = cstypes.NewExtendedHeightVoteSet(state.ChainID, height, validators) + } else { + cs.Votes = cstypes.NewHeightVoteSet(state.ChainID, height, validators) + } cs.CommitRound = -1 cs.LastValidators = state.LastValidators cs.TriggeredTimeoutPrecommit = false @@ -1921,8 +1960,12 @@ func (cs *State) finalizeCommit(ctx context.Context, height int64) { if cs.blockStore.Height() < block.Height { // NOTE: the seenCommit is local justification to commit this block, // but may differ from the LastCommit included in the next block - precommits := cs.Votes.Precommits(cs.CommitRound) - cs.blockStore.SaveBlock(block, blockParts, precommits.MakeExtendedCommit()) + seenExtendedCommit := cs.Votes.Precommits(cs.CommitRound).MakeExtendedCommit() + if cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(block.Height) { + cs.blockStore.SaveBlockWithExtendedCommit(block, blockParts, seenExtendedCommit) + } else { + cs.blockStore.SaveBlock(block, blockParts, seenExtendedCommit.ToCommit()) + } } else { // Happens during replay if we already saved the block but didn't commit logger.Debug("calling finalizeCommit on already stored block", "height", block.Height) @@ -2337,13 +2380,45 @@ func (cs *State) addVote( return } - // Verify VoteExtension if precommit and not nil - // https://github.com/tendermint/tendermint/issues/8487 - if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() { - err := cs.blockExec.VerifyVoteExtension(ctx, vote) - cs.metrics.MarkVoteExtensionReceived(err == nil) - if err != nil { - return false, err + // Check to see if the chain is configured to extend votes. + if cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(cs.Height) { + // The chain is configured to extend votes, check that the vote is + // not for a nil block and verify the extensions signature against the + // corresponding public key. + + var myAddr []byte + if cs.privValidatorPubKey != nil { + myAddr = cs.privValidatorPubKey.Address() + } + // Verify VoteExtension if precommit and not nil + // https://github.com/tendermint/tendermint/issues/8487 + if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() && + !bytes.Equal(vote.ValidatorAddress, myAddr) { // Skip the VerifyVoteExtension call if the vote was issued by this validator. + + // The core fields of the vote message were already validated in the + // consensus reactor when the vote was received. + // Here, we verify the signature of the vote extension included in the vote + // message. + _, val := cs.state.Validators.GetByIndex(vote.ValidatorIndex) + if err := vote.VerifyExtension(cs.state.ChainID, val.PubKey); err != nil { + return false, err + } + + err := cs.blockExec.VerifyVoteExtension(ctx, vote) + cs.metrics.MarkVoteExtensionReceived(err == nil) + if err != nil { + return false, err + } + } + } else { + // Vote extensions are not enabled on the network. + // strip the extension data from the vote in case any is present. + // + // TODO punish a peer if it sent a vote with an extension when the feature + // is disabled on the network. + // https://github.com/tendermint/tendermint/issues/8565 + if stripped := vote.StripExtension(); stripped { + cs.logger.Error("vote included extension data but vote extensions are not enabled", "peer", peerID) } } @@ -2492,18 +2567,18 @@ func (cs *State) signVote( // If the signedMessageType is for precommit, // use our local precommit Timeout as the max wait time for getting a singed commit. The same goes for prevote. - timeout := cs.voteTimeout(cs.Round) - + timeout := time.Second if msgType == tmproto.PrecommitType && !vote.BlockID.IsNil() { + timeout = cs.voteTimeout(cs.Round) // if the signedMessage type is for a non-nil precommit, add // VoteExtension - ext, err := cs.blockExec.ExtendVote(ctx, vote) - if err != nil { - return nil, err + if cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(cs.Height) { + ext, err := cs.blockExec.ExtendVote(ctx, vote) + if err != nil { + return nil, err + } + vote.Extension = ext } - vote.Extension = ext - } else { - timeout = time.Second } v := vote.ToProto() @@ -2543,14 +2618,17 @@ func (cs *State) signAddVote( // TODO: pass pubKey to signVote vote, err := cs.signVote(ctx, msgType, hash, header) - if err == nil { - cs.sendInternalMessage(ctx, msgInfo{&VoteMessage{vote}, "", tmtime.Now()}) - cs.logger.Debug("signed and pushed vote", "height", cs.Height, "round", cs.Round, "vote", vote) - return vote + if err != nil { + cs.logger.Error("failed signing vote", "height", cs.Height, "round", cs.Round, "vote", vote, "err", err) + return nil } - - cs.logger.Error("failed signing vote", "height", cs.Height, "round", cs.Round, "vote", vote, "err", err) - return nil + if !cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(vote.Height) { + // The signer will sign the extension, make sure to remove the data on the way out + vote.StripExtension() + } + cs.sendInternalMessage(ctx, msgInfo{&VoteMessage{vote}, "", tmtime.Now()}) + cs.logger.Debug("signed and pushed vote", "height", cs.Height, "round", cs.Round, "vote", vote) + return vote } // updatePrivValidatorPubKey get's the private validator public key and diff --git a/internal/consensus/state_test.go b/internal/consensus/state_test.go index 6fa69a1a3..797dcf59c 100644 --- a/internal/consensus/state_test.go +++ b/internal/consensus/state_test.go @@ -18,6 +18,7 @@ import ( "github.com/tendermint/tendermint/internal/eventbus" tmpubsub "github.com/tendermint/tendermint/internal/pubsub" tmquery "github.com/tendermint/tendermint/internal/pubsub/query" + "github.com/tendermint/tendermint/internal/test/factory" tmbytes "github.com/tendermint/tendermint/libs/bytes" "github.com/tendermint/tendermint/libs/log" tmrand "github.com/tendermint/tendermint/libs/rand" @@ -2026,77 +2027,101 @@ func TestFinalizeBlockCalled(t *testing.T) { } } -// TestExtendVoteCalled tests that the vote extension methods are called at the -// correct point in the consensus algorithm. -func TestExtendVoteCalled(t *testing.T) { - config := configSetup(t) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() +// TestExtendVoteCalledWhenEnabled tests that the vote extension methods are called at the +// correct point in the consensus algorithm when vote extensions are enabled. +func TestExtendVoteCalledWhenEnabled(t *testing.T) { + for _, testCase := range []struct { + name string + enabled bool + }{ + { + name: "enabled", + enabled: true, + }, + { + name: "disabled", + enabled: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + config := configSetup(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() - m := abcimocks.NewApplication(t) - m.On("ProcessProposal", mock.Anything, mock.Anything).Return(&abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil) - m.On("PrepareProposal", mock.Anything, mock.Anything).Return(&abci.ResponsePrepareProposal{}, nil) - m.On("ExtendVote", mock.Anything, mock.Anything).Return(&abci.ResponseExtendVote{ - VoteExtension: []byte("extension"), - }, nil) - m.On("VerifyVoteExtension", mock.Anything, mock.Anything).Return(&abci.ResponseVerifyVoteExtension{ - Status: abci.ResponseVerifyVoteExtension_ACCEPT, - }, nil) - m.On("Commit", mock.Anything).Return(&abci.ResponseCommit{}, nil).Maybe() - m.On("FinalizeBlock", mock.Anything, mock.Anything).Return(&abci.ResponseFinalizeBlock{}, nil).Maybe() - cs1, vss := makeState(ctx, t, makeStateArgs{config: config, application: m}) - height, round := cs1.Height, cs1.Round + m := abcimocks.NewApplication(t) + m.On("ProcessProposal", mock.Anything, mock.Anything).Return(&abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}, nil) + m.On("PrepareProposal", mock.Anything, mock.Anything).Return(&abci.ResponsePrepareProposal{}, nil) + if testCase.enabled { + m.On("ExtendVote", mock.Anything, mock.Anything).Return(&abci.ResponseExtendVote{ + VoteExtension: []byte("extension"), + }, nil) + m.On("VerifyVoteExtension", mock.Anything, mock.Anything).Return(&abci.ResponseVerifyVoteExtension{ + Status: abci.ResponseVerifyVoteExtension_ACCEPT, + }, nil) + } + m.On("Commit", mock.Anything).Return(&abci.ResponseCommit{}, nil).Maybe() + m.On("FinalizeBlock", mock.Anything, mock.Anything).Return(&abci.ResponseFinalizeBlock{}, nil).Maybe() + c := factory.ConsensusParams() + if !testCase.enabled { + c.ABCI.VoteExtensionsEnableHeight = 0 + } + cs1, vss := makeState(ctx, t, makeStateArgs{config: config, application: m, consensusParams: c}) + height, round := cs1.Height, cs1.Round - proposalCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryCompleteProposal) - newRoundCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryNewRound) - pv1, err := cs1.privValidator.GetPubKey(ctx) - require.NoError(t, err) - addr := pv1.Address() - voteCh := subscribeToVoter(ctx, t, cs1, addr) + proposalCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryCompleteProposal) + newRoundCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryNewRound) + pv1, err := cs1.privValidator.GetPubKey(ctx) + require.NoError(t, err) + addr := pv1.Address() + voteCh := subscribeToVoter(ctx, t, cs1, addr) - startTestRound(ctx, cs1, cs1.Height, round) - ensureNewRound(t, newRoundCh, height, round) - ensureNewProposal(t, proposalCh, height, round) + startTestRound(ctx, cs1, cs1.Height, round) + ensureNewRound(t, newRoundCh, height, round) + ensureNewProposal(t, proposalCh, height, round) - m.AssertNotCalled(t, "ExtendVote", mock.Anything, mock.Anything) + m.AssertNotCalled(t, "ExtendVote", mock.Anything, mock.Anything) - rs := cs1.GetRoundState() + rs := cs1.GetRoundState() - blockID := types.BlockID{ - Hash: rs.ProposalBlock.Hash(), - PartSetHeader: rs.ProposalBlockParts.Header(), - } - signAddVotes(ctx, t, cs1, tmproto.PrevoteType, config.ChainID(), blockID, vss[1:]...) - ensurePrevoteMatch(t, voteCh, height, round, blockID.Hash) + blockID := types.BlockID{ + Hash: rs.ProposalBlock.Hash(), + PartSetHeader: rs.ProposalBlockParts.Header(), + } + signAddVotes(ctx, t, cs1, tmproto.PrevoteType, config.ChainID(), blockID, vss[1:]...) + ensurePrevoteMatch(t, voteCh, height, round, blockID.Hash) - ensurePrecommit(t, voteCh, height, round) + ensurePrecommit(t, voteCh, height, round) - m.AssertCalled(t, "ExtendVote", ctx, &abci.RequestExtendVote{ - Height: height, - Hash: blockID.Hash, - }) + if testCase.enabled { + m.AssertCalled(t, "ExtendVote", ctx, &abci.RequestExtendVote{ + Height: height, + Hash: blockID.Hash, + }) + } else { + m.AssertNotCalled(t, "ExtendVote", mock.Anything, mock.Anything) + } - m.AssertCalled(t, "VerifyVoteExtension", ctx, &abci.RequestVerifyVoteExtension{ - Hash: blockID.Hash, - ValidatorAddress: addr, - Height: height, - VoteExtension: []byte("extension"), - }) - signAddVotes(ctx, t, cs1, tmproto.PrecommitType, config.ChainID(), blockID, vss[1:]...) - ensureNewRound(t, newRoundCh, height+1, 0) - m.AssertExpectations(t) + signAddVotes(ctx, t, cs1, tmproto.PrecommitType, config.ChainID(), blockID, vss[1:]...) + ensureNewRound(t, newRoundCh, height+1, 0) + m.AssertExpectations(t) - // Only 3 of the vote extensions are seen, as consensus proceeds as soon as the +2/3 threshold - // is observed by the consensus engine. - for _, pv := range vss[:3] { - pv, err := pv.GetPubKey(ctx) - require.NoError(t, err) - addr := pv.Address() - m.AssertCalled(t, "VerifyVoteExtension", ctx, &abci.RequestVerifyVoteExtension{ - Hash: blockID.Hash, - ValidatorAddress: addr, - Height: height, - VoteExtension: []byte("extension"), + // Only 3 of the vote extensions are seen, as consensus proceeds as soon as the +2/3 threshold + // is observed by the consensus engine. + for _, pv := range vss[1:3] { + pv, err := pv.GetPubKey(ctx) + require.NoError(t, err) + addr := pv.Address() + if testCase.enabled { + m.AssertCalled(t, "VerifyVoteExtension", ctx, &abci.RequestVerifyVoteExtension{ + Hash: blockID.Hash, + ValidatorAddress: addr, + Height: height, + VoteExtension: []byte("extension"), + }) + } else { + m.AssertNotCalled(t, "VerifyVoteExtension", mock.Anything, mock.Anything) + } + } }) } @@ -2121,6 +2146,7 @@ func TestVerifyVoteExtensionNotCalledOnAbsentPrecommit(t *testing.T) { m.On("FinalizeBlock", mock.Anything, mock.Anything).Return(&abci.ResponseFinalizeBlock{}, nil).Maybe() cs1, vss := makeState(ctx, t, makeStateArgs{config: config, application: m}) height, round := cs1.Height, cs1.Round + cs1.state.ConsensusParams.ABCI.VoteExtensionsEnableHeight = cs1.Height proposalCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryCompleteProposal) newRoundCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryNewRound) @@ -2138,7 +2164,7 @@ func TestVerifyVoteExtensionNotCalledOnAbsentPrecommit(t *testing.T) { Hash: rs.ProposalBlock.Hash(), PartSetHeader: rs.ProposalBlockParts.Header(), } - signAddVotes(ctx, t, cs1, tmproto.PrevoteType, config.ChainID(), blockID, vss[2:]...) + signAddVotes(ctx, t, cs1, tmproto.PrevoteType, config.ChainID(), blockID, vss...) ensurePrevoteMatch(t, voteCh, height, round, blockID.Hash) ensurePrecommit(t, voteCh, height, round) @@ -2148,13 +2174,6 @@ func TestVerifyVoteExtensionNotCalledOnAbsentPrecommit(t *testing.T) { Hash: blockID.Hash, }) - m.AssertCalled(t, "VerifyVoteExtension", mock.Anything, &abci.RequestVerifyVoteExtension{ - Hash: blockID.Hash, - ValidatorAddress: addr, - Height: height, - VoteExtension: []byte("extension"), - }) - m.On("Commit", mock.Anything).Return(&abci.ResponseCommit{}, nil).Maybe() signAddVotes(ctx, t, cs1, tmproto.PrecommitType, config.ChainID(), blockID, vss[2:]...) ensureNewRound(t, newRoundCh, height+1, 0) @@ -2266,6 +2285,134 @@ func TestPrepareProposalReceivesVoteExtensions(t *testing.T) { } } +// TestVoteExtensionEnableHeight tests that 'ExtensionRequireHeight' correctly +// enforces that vote extensions be present in consensus for heights greater than +// or equal to the configured value. +func TestVoteExtensionEnableHeight(t *testing.T) { + for _, testCase := range []struct { + name string + enableHeight int64 + hasExtension bool + expectExtendCalled bool + expectVerifyCalled bool + expectSuccessfulRound bool + }{ + { + name: "extension present but not enabled", + hasExtension: true, + enableHeight: 0, + expectExtendCalled: false, + expectVerifyCalled: false, + expectSuccessfulRound: true, + }, + { + name: "extension absent but not required", + hasExtension: false, + enableHeight: 0, + expectExtendCalled: false, + expectVerifyCalled: false, + expectSuccessfulRound: true, + }, + { + name: "extension present and required", + hasExtension: true, + enableHeight: 1, + expectExtendCalled: true, + expectVerifyCalled: true, + expectSuccessfulRound: true, + }, + { + name: "extension absent but required", + hasExtension: false, + enableHeight: 1, + expectExtendCalled: true, + expectVerifyCalled: false, + expectSuccessfulRound: false, + }, + { + name: "extension absent but required in future height", + hasExtension: false, + enableHeight: 2, + expectExtendCalled: false, + expectVerifyCalled: false, + expectSuccessfulRound: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + config := configSetup(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + numValidators := 3 + m := abcimocks.NewApplication(t) + m.On("ProcessProposal", mock.Anything, mock.Anything).Return(&abci.ResponseProcessProposal{ + Status: abci.ResponseProcessProposal_ACCEPT, + }, nil) + m.On("PrepareProposal", mock.Anything, mock.Anything).Return(&abci.ResponsePrepareProposal{}, nil) + if testCase.expectExtendCalled { + m.On("ExtendVote", mock.Anything, mock.Anything).Return(&abci.ResponseExtendVote{}, nil) + } + if testCase.expectVerifyCalled { + m.On("VerifyVoteExtension", mock.Anything, mock.Anything).Return(&abci.ResponseVerifyVoteExtension{ + Status: abci.ResponseVerifyVoteExtension_ACCEPT, + }, nil).Times(numValidators - 1) + } + m.On("FinalizeBlock", mock.Anything, mock.Anything).Return(&abci.ResponseFinalizeBlock{}, nil).Maybe() + m.On("Commit", mock.Anything).Return(&abci.ResponseCommit{}, nil).Maybe() + c := factory.ConsensusParams() + c.ABCI.VoteExtensionsEnableHeight = testCase.enableHeight + cs1, vss := makeState(ctx, t, makeStateArgs{config: config, application: m, validators: numValidators, consensusParams: c}) + cs1.state.ConsensusParams.ABCI.VoteExtensionsEnableHeight = testCase.enableHeight + height, round := cs1.Height, cs1.Round + + timeoutCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryTimeoutPropose) + proposalCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryCompleteProposal) + newRoundCh := subscribe(ctx, t, cs1.eventBus, types.EventQueryNewRound) + pv1, err := cs1.privValidator.GetPubKey(ctx) + require.NoError(t, err) + addr := pv1.Address() + voteCh := subscribeToVoter(ctx, t, cs1, addr) + + startTestRound(ctx, cs1, cs1.Height, round) + ensureNewRound(t, newRoundCh, height, round) + ensureNewProposal(t, proposalCh, height, round) + rs := cs1.GetRoundState() + + blockID := types.BlockID{ + Hash: rs.ProposalBlock.Hash(), + PartSetHeader: rs.ProposalBlockParts.Header(), + } + + // sign all of the votes + signAddVotes(ctx, t, cs1, tmproto.PrevoteType, config.ChainID(), blockID, vss[1:]...) + ensurePrevoteMatch(t, voteCh, height, round, rs.ProposalBlock.Hash()) + + var ext []byte + if testCase.hasExtension { + ext = []byte("extension") + } + + for _, vs := range vss[1:] { + vote, err := vs.signVote(ctx, tmproto.PrecommitType, config.ChainID(), blockID, ext) + if !testCase.hasExtension { + vote.ExtensionSignature = nil + } + require.NoError(t, err) + addVotes(cs1, vote) + } + if testCase.expectSuccessfulRound { + ensurePrecommit(t, voteCh, height, round) + height++ + ensureNewRound(t, newRoundCh, height, round) + } else { + ensureNoNewTimeout(t, timeoutCh, cs1.state.ConsensusParams.Timeout.VoteTimeout(round).Nanoseconds()) + } + + m.AssertExpectations(t) + }) + } +} + // 4 vals, 3 Nil Precommits at P0 // What we want: // P0 waits for timeoutPrecommit before starting next round diff --git a/internal/consensus/types/height_vote_set.go b/internal/consensus/types/height_vote_set.go index 661c5120e..389c02356 100644 --- a/internal/consensus/types/height_vote_set.go +++ b/internal/consensus/types/height_vote_set.go @@ -38,9 +38,10 @@ We let each peer provide us with up to 2 unexpected "catchup" rounds. One for their LastCommit round, and another for the official commit round. */ type HeightVoteSet struct { - chainID string - height int64 - valSet *types.ValidatorSet + chainID string + height int64 + valSet *types.ValidatorSet + extensionsEnabled bool mtx sync.Mutex round int32 // max tracked round @@ -50,7 +51,17 @@ type HeightVoteSet struct { func NewHeightVoteSet(chainID string, height int64, valSet *types.ValidatorSet) *HeightVoteSet { hvs := &HeightVoteSet{ - chainID: chainID, + chainID: chainID, + extensionsEnabled: false, + } + hvs.Reset(height, valSet) + return hvs +} + +func NewExtendedHeightVoteSet(chainID string, height int64, valSet *types.ValidatorSet) *HeightVoteSet { + hvs := &HeightVoteSet{ + chainID: chainID, + extensionsEnabled: true, } hvs.Reset(height, valSet) return hvs @@ -108,7 +119,12 @@ func (hvs *HeightVoteSet) addRound(round int32) { } // log.Debug("addRound(round)", "round", round) prevotes := types.NewVoteSet(hvs.chainID, hvs.height, round, tmproto.PrevoteType, hvs.valSet) - precommits := types.NewVoteSet(hvs.chainID, hvs.height, round, tmproto.PrecommitType, hvs.valSet) + var precommits *types.VoteSet + if hvs.extensionsEnabled { + precommits = types.NewExtendedVoteSet(hvs.chainID, hvs.height, round, tmproto.PrecommitType, hvs.valSet) + } else { + precommits = types.NewVoteSet(hvs.chainID, hvs.height, round, tmproto.PrecommitType, hvs.valSet) + } hvs.roundVoteSets[round] = RoundVoteSet{ Prevotes: prevotes, Precommits: precommits, diff --git a/internal/consensus/types/height_vote_set_test.go b/internal/consensus/types/height_vote_set_test.go index acffa794c..a2cfd84ec 100644 --- a/internal/consensus/types/height_vote_set_test.go +++ b/internal/consensus/types/height_vote_set_test.go @@ -27,7 +27,7 @@ func TestPeerCatchupRounds(t *testing.T) { valSet, privVals := factory.ValidatorSet(ctx, t, 10, 1) chainID := cfg.ChainID() - hvs := NewHeightVoteSet(chainID, 1, valSet) + hvs := NewExtendedHeightVoteSet(chainID, 1, valSet) vote999_0 := makeVoteHR(ctx, t, 1, 0, 999, privVals, chainID) added, err := hvs.AddVote(vote999_0, "peer1") diff --git a/internal/evidence/pool_test.go b/internal/evidence/pool_test.go index 4047d3e7f..f80d13c7b 100644 --- a/internal/evidence/pool_test.go +++ b/internal/evidence/pool_test.go @@ -250,7 +250,7 @@ func TestEvidencePoolUpdate(t *testing.T) { ) require.NoError(t, err) lastExtCommit := makeExtCommit(height, val.PrivKey.PubKey().Address()) - block := types.MakeBlock(height+1, []types.Tx{}, lastExtCommit.StripExtensions(), []types.Evidence{ev}) + block := types.MakeBlock(height+1, []types.Tx{}, lastExtCommit.ToCommit(), []types.Evidence{ev}) // update state (partially) state.LastBlockHeight = height + 1 @@ -569,7 +569,7 @@ func initializeBlockStore(db dbm.DB, state sm.State, valAddr []byte) (*store.Blo for i := int64(1); i <= state.LastBlockHeight; i++ { lastCommit := makeExtCommit(i-1, valAddr) - block := sf.MakeBlock(state, i, lastCommit.StripExtensions()) + block := sf.MakeBlock(state, i, lastCommit.ToCommit()) block.Header.Time = defaultEvidenceTime.Add(time.Duration(i) * time.Minute) block.Header.Version = version.Consensus{Block: version.BlockProtocol, App: 1} @@ -580,7 +580,7 @@ func initializeBlockStore(db dbm.DB, state sm.State, valAddr []byte) (*store.Blo } seenCommit := makeExtCommit(i, valAddr) - blockStore.SaveBlock(block, partSet, seenCommit) + blockStore.SaveBlockWithExtendedCommit(block, partSet, seenCommit) } return blockStore, nil @@ -596,6 +596,7 @@ func makeExtCommit(height int64, valAddr []byte) *types.ExtendedCommit { Timestamp: defaultEvidenceTime, Signature: []byte("Signature"), }, + ExtensionSignature: []byte("Extended Signature"), }}, } } diff --git a/internal/evidence/verify_test.go b/internal/evidence/verify_test.go index 2ed84fa69..6341fde1d 100644 --- a/internal/evidence/verify_test.go +++ b/internal/evidence/verify_test.go @@ -233,10 +233,10 @@ func TestVerifyLightClientAttack_Equivocation(t *testing.T) { // we are simulating a duplicate vote attack where all the validators in the conflictingVals set // except the last validator vote twice blockID := factory.MakeBlockIDWithHash(conflictingHeader.Hash()) - voteSet := types.NewVoteSet(evidenceChainID, 10, 1, tmproto.SignedMsgType(2), conflictingVals) + voteSet := types.NewExtendedVoteSet(evidenceChainID, 10, 1, tmproto.SignedMsgType(2), conflictingVals) extCommit, err := factory.MakeExtendedCommit(ctx, blockID, 10, 1, voteSet, conflictingPrivVals[:4], defaultEvidenceTime) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() ev := &types.LightClientAttackEvidence{ ConflictingBlock: &types.LightBlock{ @@ -253,11 +253,11 @@ func TestVerifyLightClientAttack_Equivocation(t *testing.T) { } trustedBlockID := makeBlockID(trustedHeader.Hash(), 1000, []byte("partshash")) - trustedVoteSet := types.NewVoteSet(evidenceChainID, 10, 1, tmproto.SignedMsgType(2), conflictingVals) + trustedVoteSet := types.NewExtendedVoteSet(evidenceChainID, 10, 1, tmproto.SignedMsgType(2), conflictingVals) trustedExtCommit, err := factory.MakeExtendedCommit(ctx, trustedBlockID, 10, 1, trustedVoteSet, conflictingPrivVals, defaultEvidenceTime) require.NoError(t, err) - trustedCommit := trustedExtCommit.StripExtensions() + trustedCommit := trustedExtCommit.ToCommit() trustedSignedHeader := &types.SignedHeader{ Header: trustedHeader, @@ -336,10 +336,10 @@ func TestVerifyLightClientAttack_Amnesia(t *testing.T) { // we are simulating an amnesia attack where all the validators in the conflictingVals set // except the last validator vote twice. However this time the commits are of different rounds. blockID := makeBlockID(conflictingHeader.Hash(), 1000, []byte("partshash")) - voteSet := types.NewVoteSet(evidenceChainID, height, 0, tmproto.SignedMsgType(2), conflictingVals) + voteSet := types.NewExtendedVoteSet(evidenceChainID, height, 0, tmproto.SignedMsgType(2), conflictingVals) extCommit, err := factory.MakeExtendedCommit(ctx, blockID, height, 0, voteSet, conflictingPrivVals, defaultEvidenceTime) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() ev := &types.LightClientAttackEvidence{ ConflictingBlock: &types.LightBlock{ @@ -356,11 +356,11 @@ func TestVerifyLightClientAttack_Amnesia(t *testing.T) { } trustedBlockID := makeBlockID(trustedHeader.Hash(), 1000, []byte("partshash")) - trustedVoteSet := types.NewVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), conflictingVals) + trustedVoteSet := types.NewExtendedVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), conflictingVals) trustedExtCommit, err := factory.MakeExtendedCommit(ctx, trustedBlockID, height, 1, trustedVoteSet, conflictingPrivVals, defaultEvidenceTime) require.NoError(t, err) - trustedCommit := trustedExtCommit.StripExtensions() + trustedCommit := trustedExtCommit.ToCommit() trustedSignedHeader := &types.SignedHeader{ Header: trustedHeader, @@ -553,10 +553,10 @@ func makeLunaticEvidence( }) blockID := factory.MakeBlockIDWithHash(conflictingHeader.Hash()) - voteSet := types.NewVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), conflictingVals) + voteSet := types.NewExtendedVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), conflictingVals) extCommit, err := factory.MakeExtendedCommit(ctx, blockID, height, 1, voteSet, conflictingPrivVals, defaultEvidenceTime) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() ev = &types.LightClientAttackEvidence{ ConflictingBlock: &types.LightBlock{ @@ -582,10 +582,10 @@ func makeLunaticEvidence( } trustedBlockID := factory.MakeBlockIDWithHash(trustedHeader.Hash()) trustedVals, privVals := factory.ValidatorSet(ctx, t, totalVals, defaultVotingPower) - trustedVoteSet := types.NewVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), trustedVals) + trustedVoteSet := types.NewExtendedVoteSet(evidenceChainID, height, 1, tmproto.SignedMsgType(2), trustedVals) trustedExtCommit, err := factory.MakeExtendedCommit(ctx, trustedBlockID, height, 1, trustedVoteSet, privVals, defaultEvidenceTime) require.NoError(t, err) - trustedCommit := trustedExtCommit.StripExtensions() + trustedCommit := trustedExtCommit.ToCommit() trusted = &types.LightBlock{ SignedHeader: &types.SignedHeader{ diff --git a/internal/inspect/rpc/rpc.go b/internal/inspect/rpc/rpc.go index 00c3e52ef..d70616834 100644 --- a/internal/inspect/rpc/rpc.go +++ b/internal/inspect/rpc/rpc.go @@ -125,7 +125,8 @@ func serverRPCConfig(r *config.RPCConfig) *server.Config { // If necessary adjust global WriteTimeout to ensure it's greater than // TimeoutBroadcastTxCommit. // See https://github.com/tendermint/tendermint/issues/3435 - if cfg.WriteTimeout <= r.TimeoutBroadcastTxCommit { + // Note we don't need to adjust anything if the timeout is already unlimited. + if cfg.WriteTimeout > 0 && cfg.WriteTimeout <= r.TimeoutBroadcastTxCommit { cfg.WriteTimeout = r.TimeoutBroadcastTxCommit + 1*time.Second } return cfg diff --git a/internal/mempool/reactor.go b/internal/mempool/reactor.go index 808c90a7c..62cdf386c 100644 --- a/internal/mempool/reactor.go +++ b/internal/mempool/reactor.go @@ -6,7 +6,6 @@ import ( "fmt" "runtime/debug" "sync" - "time" "github.com/tendermint/tendermint/config" "github.com/tendermint/tendermint/internal/libs/clist" @@ -22,13 +21,6 @@ var ( _ p2p.Wrapper = (*protomem.Message)(nil) ) -// PeerManager defines the interface contract required for getting necessary -// peer information. This should eventually be replaced with a message-oriented -// approach utilizing the p2p stack. -type PeerManager interface { - GetHeight(types.NodeID) int64 -} - // Reactor implements a service that contains mempool of txs that are broadcasted // amongst peers. It maintains a map from peer ID to counter, to prevent gossiping // txs to the peers you received it from. @@ -40,9 +32,8 @@ type Reactor struct { mempool *TxMempool ids *IDs - getPeerHeight func(types.NodeID) int64 - peerEvents p2p.PeerEventSubscriber - chCreator p2p.ChannelCreator + peerEvents p2p.PeerEventSubscriber + chCreator p2p.ChannelCreator // observePanic is a function for observing panics that were recovered in methods on // Reactor. observePanic is called with the recovered value. @@ -59,18 +50,16 @@ func NewReactor( txmp *TxMempool, chCreator p2p.ChannelCreator, peerEvents p2p.PeerEventSubscriber, - getPeerHeight func(types.NodeID) int64, ) *Reactor { r := &Reactor{ - logger: logger, - cfg: cfg, - mempool: txmp, - ids: NewMempoolIDs(), - chCreator: chCreator, - peerEvents: peerEvents, - getPeerHeight: getPeerHeight, - peerRoutines: make(map[types.NodeID]context.CancelFunc), - observePanic: defaultObservePanic, + logger: logger, + cfg: cfg, + mempool: txmp, + ids: NewMempoolIDs(), + chCreator: chCreator, + peerEvents: peerEvents, + peerRoutines: make(map[types.NodeID]context.CancelFunc), + observePanic: defaultObservePanic, } r.BaseService = *service.NewBaseService(logger, "Mempool", r) @@ -327,15 +316,6 @@ func (r *Reactor) broadcastTxRoutine(ctx context.Context, peerID types.NodeID, m memTx := nextGossipTx.Value.(*WrappedTx) - if r.getPeerHeight != nil { - height := r.getPeerHeight(peerID) - if height > 0 && height < memTx.height-1 { - // allow for a lag of one block - time.Sleep(PeerCatchupSleepIntervalMS * time.Millisecond) - continue - } - } - // NOTE: Transaction batching was disabled due to: // https://github.com/tendermint/tendermint/issues/5796 if ok := r.mempool.txStore.TxHasPeer(memTx.hash, peerMempoolID); !ok { diff --git a/internal/mempool/reactor_test.go b/internal/mempool/reactor_test.go index 9bc305af5..f67b10307 100644 --- a/internal/mempool/reactor_test.go +++ b/internal/mempool/reactor_test.go @@ -85,7 +85,6 @@ func setupReactors(ctx context.Context, t *testing.T, logger log.Logger, numNode mempool, chCreator, func(ctx context.Context) *p2p.PeerUpdates { return rts.peerUpdates[nodeID] }, - rts.network.Nodes[nodeID].PeerManager.GetHeight, ) rts.nodes = append(rts.nodes, nodeID) diff --git a/internal/p2p/peermanager.go b/internal/p2p/peermanager.go index 756551a49..165b00e61 100644 --- a/internal/p2p/peermanager.go +++ b/internal/p2p/peermanager.go @@ -1027,37 +1027,6 @@ func (m *PeerManager) retryDelay(failures uint32, persistent bool) time.Duration return delay } -// GetHeight returns a peer's height, as reported via SetHeight, or 0 if the -// peer or height is unknown. -// -// FIXME: This is a temporary workaround to share state between the consensus -// and mempool reactors, carried over from the legacy P2P stack. Reactors should -// not have dependencies on each other, instead tracking this themselves. -func (m *PeerManager) GetHeight(peerID types.NodeID) int64 { - m.mtx.Lock() - defer m.mtx.Unlock() - - peer, _ := m.store.Get(peerID) - return peer.Height -} - -// SetHeight stores a peer's height, making it available via GetHeight. -// -// FIXME: This is a temporary workaround to share state between the consensus -// and mempool reactors, carried over from the legacy P2P stack. Reactors should -// not have dependencies on each other, instead tracking this themselves. -func (m *PeerManager) SetHeight(peerID types.NodeID, height int64) error { - m.mtx.Lock() - defer m.mtx.Unlock() - - peer, ok := m.store.Get(peerID) - if !ok { - peer = m.newPeerInfo(peerID) - } - peer.Height = height - return m.store.Set(peer) -} - // peerStore stores information about peers. It is not thread-safe, assuming it // is only used by PeerManager which handles concurrency control. This allows // the manager to execute multiple operations atomically via its own mutex. diff --git a/internal/p2p/peermanager_test.go b/internal/p2p/peermanager_test.go index 82d1e2693..47e8462a4 100644 --- a/internal/p2p/peermanager_test.go +++ b/internal/p2p/peermanager_test.go @@ -1868,38 +1868,3 @@ func TestPeerManager_Advertise_Self(t *testing.T) { self, }, peerManager.Advertise(dID, 100)) } - -func TestPeerManager_SetHeight_GetHeight(t *testing.T) { - a := p2p.NodeAddress{Protocol: "memory", NodeID: types.NodeID(strings.Repeat("a", 40))} - b := p2p.NodeAddress{Protocol: "memory", NodeID: types.NodeID(strings.Repeat("b", 40))} - - db := dbm.NewMemDB() - peerManager, err := p2p.NewPeerManager(selfID, db, p2p.PeerManagerOptions{}) - require.NoError(t, err) - - // Getting a height should default to 0, for unknown peers and - // for known peers without height. - added, err := peerManager.Add(a) - require.NoError(t, err) - require.True(t, added) - require.EqualValues(t, 0, peerManager.GetHeight(a.NodeID)) - require.EqualValues(t, 0, peerManager.GetHeight(b.NodeID)) - - // Setting a height should work for a known node. - require.NoError(t, peerManager.SetHeight(a.NodeID, 3)) - require.EqualValues(t, 3, peerManager.GetHeight(a.NodeID)) - - // Setting a height should add an unknown node. - require.Equal(t, []types.NodeID{a.NodeID}, peerManager.Peers()) - require.NoError(t, peerManager.SetHeight(b.NodeID, 7)) - require.EqualValues(t, 7, peerManager.GetHeight(b.NodeID)) - require.ElementsMatch(t, []types.NodeID{a.NodeID, b.NodeID}, peerManager.Peers()) - - // The heights should not be persisted. - peerManager, err = p2p.NewPeerManager(selfID, db, p2p.PeerManagerOptions{}) - require.NoError(t, err) - - require.ElementsMatch(t, []types.NodeID{a.NodeID, b.NodeID}, peerManager.Peers()) - require.Zero(t, peerManager.GetHeight(a.NodeID)) - require.Zero(t, peerManager.GetHeight(b.NodeID)) -} diff --git a/internal/proxy/client_test.go b/internal/proxy/client_test.go index 09ac3f2c8..41a34bde7 100644 --- a/internal/proxy/client_test.go +++ b/internal/proxy/client_test.go @@ -58,7 +58,7 @@ func (app *appConnTest) Info(ctx context.Context, req *types.RequestInfo) (*type var SOCKET = "socket" func TestEcho(t *testing.T) { - sockPath := fmt.Sprintf("unix:///tmp/echo_%v.sock", tmrand.Str(6)) + sockPath := fmt.Sprintf("unix://%s/echo_%v.sock", t.TempDir(), tmrand.Str(6)) logger := log.NewNopLogger() client, err := abciclient.NewClient(logger, sockPath, SOCKET, true) if err != nil { @@ -98,7 +98,7 @@ func TestEcho(t *testing.T) { func BenchmarkEcho(b *testing.B) { b.StopTimer() // Initialize - sockPath := fmt.Sprintf("unix:///tmp/echo_%v.sock", tmrand.Str(6)) + sockPath := fmt.Sprintf("unix://%s/echo_%v.sock", b.TempDir(), tmrand.Str(6)) logger := log.NewNopLogger() client, err := abciclient.NewClient(logger, sockPath, SOCKET, true) if err != nil { @@ -146,7 +146,7 @@ func TestInfo(t *testing.T) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() - sockPath := fmt.Sprintf("unix:///tmp/echo_%v.sock", tmrand.Str(6)) + sockPath := fmt.Sprintf("unix://%s/echo_%v.sock", t.TempDir(), tmrand.Str(6)) logger := log.NewNopLogger() client, err := abciclient.NewClient(logger, sockPath, SOCKET, true) if err != nil { diff --git a/internal/rpc/core/env.go b/internal/rpc/core/env.go index 24f43a4a7..124525f26 100644 --- a/internal/rpc/core/env.go +++ b/internal/rpc/core/env.go @@ -236,7 +236,8 @@ func (env *Environment) StartService(ctx context.Context, conf *config.Config) ( // If necessary adjust global WriteTimeout to ensure it's greater than // TimeoutBroadcastTxCommit. // See https://github.com/tendermint/tendermint/issues/3435 - if cfg.WriteTimeout <= conf.RPC.TimeoutBroadcastTxCommit { + // Note we don't need to adjust anything if the timeout is already unlimited. + if cfg.WriteTimeout > 0 && cfg.WriteTimeout <= conf.RPC.TimeoutBroadcastTxCommit { cfg.WriteTimeout = conf.RPC.TimeoutBroadcastTxCommit + 1*time.Second } diff --git a/internal/rpc/core/routes.go b/internal/rpc/core/routes.go index 4bc1ca414..cafb92094 100644 --- a/internal/rpc/core/routes.go +++ b/internal/rpc/core/routes.go @@ -28,7 +28,7 @@ func NewRoutesMap(svc RPCService, opts *RouteOptions) RoutesMap { out := RoutesMap{ // Event subscription. Note that subscribe, unsubscribe, and // unsubscribe_all are only available via the websocket endpoint. - "events": rpc.NewRPCFunc(svc.Events), + "events": rpc.NewRPCFunc(svc.Events).Timeout(0), "subscribe": rpc.NewWSRPCFunc(svc.Subscribe), "unsubscribe": rpc.NewWSRPCFunc(svc.Unsubscribe), "unsubscribe_all": rpc.NewWSRPCFunc(svc.UnsubscribeAll), diff --git a/internal/state/execution.go b/internal/state/execution.go index 2c88c793b..47c2cb7ae 100644 --- a/internal/state/execution.go +++ b/internal/state/execution.go @@ -3,6 +3,7 @@ package state import ( "bytes" "context" + "errors" "fmt" "time" @@ -100,15 +101,14 @@ func (blockExec *BlockExecutor) CreateProposalBlock( maxDataBytes := types.MaxDataBytes(maxBytes, evSize, state.Validators.Size()) txs := blockExec.mempool.ReapMaxBytesMaxGas(maxDataBytes, maxGas) - commit := lastExtCommit.StripExtensions() + commit := lastExtCommit.ToCommit() block := state.MakeBlock(height, txs, commit, evidence, proposerAddr) - rpp, err := blockExec.appClient.PrepareProposal( ctx, &abci.RequestPrepareProposal{ MaxTxBytes: maxDataBytes, Txs: block.Txs.ToSliceOfBytes(), - LocalLastCommit: buildExtendedCommitInfo(lastExtCommit, blockExec.store, state.InitialHeight), + LocalLastCommit: buildExtendedCommitInfo(lastExtCommit, blockExec.store, state.InitialHeight, state.ConsensusParams.ABCI), ByzantineValidators: block.Evidence.ToABCI(), Height: block.Height, Time: block.Time, @@ -321,7 +321,7 @@ func (blockExec *BlockExecutor) VerifyVoteExtension(ctx context.Context, vote *t } if !resp.IsOK() { - return types.ErrVoteInvalidExtension + return errors.New("invalid vote extension") } return nil @@ -428,7 +428,7 @@ func buildLastCommitInfo(block *types.Block, store Store, initialHeight int64) a // data, it returns an empty record. // // Assumes that the commit signatures are sorted according to validator index. -func buildExtendedCommitInfo(ec *types.ExtendedCommit, store Store, initialHeight int64) abci.ExtendedCommitInfo { +func buildExtendedCommitInfo(ec *types.ExtendedCommit, store Store, initialHeight int64, ap types.ABCIParams) abci.ExtendedCommitInfo { if ec.Height < initialHeight { // There are no extended commits for heights below the initial height. return abci.ExtendedCommitInfo{} @@ -466,9 +466,15 @@ func buildExtendedCommitInfo(ec *types.ExtendedCommit, store Store, initialHeigh } var ext []byte - if ecs.BlockIDFlag == types.BlockIDFlagCommit { - // We only care about vote extensions if a validator has voted to - // commit. + // Check if vote extensions were enabled during the commit's height: ec.Height. + // ec is the commit from the previous height, so if extensions were enabled + // during that height, we ensure they are present and deliver the data to + // the proposer. If they were not enabled during this previous height, we + // will not deliver extension data. + if ap.VoteExtensionsEnabled(ec.Height) && ecs.BlockIDFlag == types.BlockIDFlagCommit { + if err := ecs.EnsureExtension(); err != nil { + panic(fmt.Errorf("commit at height %d received with missing vote extensions data", ec.Height)) + } ext = ecs.Extension } diff --git a/internal/state/execution_test.go b/internal/state/execution_test.go index ffe9cb6f8..5fb4dc297 100644 --- a/internal/state/execution_test.go +++ b/internal/state/execution_test.go @@ -140,7 +140,7 @@ func TestFinalizeBlockDecidedLastCommit(t *testing.T) { } // block for height 2 - block := sf.MakeBlock(state, 2, lastCommit.StripExtensions()) + block := sf.MakeBlock(state, 2, lastCommit.ToCommit()) bps, err := block.MakePartSet(testPartSize) require.NoError(t, err) blockID := types.BlockID{Hash: block.Hash(), PartSetHeader: bps.Header()} @@ -1004,6 +1004,116 @@ func TestPrepareProposalErrorOnPrepareProposalError(t *testing.T) { mp.AssertExpectations(t) } +// TestCreateProposalBlockPanicOnAbsentVoteExtensions ensures that the CreateProposalBlock +// call correctly panics when the vote extension data is missing from the extended commit +// data that the method receives. +func TestCreateProposalAbsentVoteExtensions(t *testing.T) { + for _, testCase := range []struct { + name string + + // The height that is about to be proposed + height int64 + + // The first height during which vote extensions will be required for consensus to proceed. + extensionEnableHeight int64 + expectPanic bool + }{ + { + name: "missing extension data on first required height", + height: 2, + extensionEnableHeight: 1, + expectPanic: true, + }, + { + name: "missing extension during before required height", + height: 2, + extensionEnableHeight: 2, + expectPanic: false, + }, + { + name: "missing extension data and not required", + height: 2, + extensionEnableHeight: 0, + expectPanic: false, + }, + { + name: "missing extension data and required in two heights", + height: 2, + extensionEnableHeight: 3, + expectPanic: false, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + logger := log.NewNopLogger() + + eventBus := eventbus.NewDefault(logger) + require.NoError(t, eventBus.Start(ctx)) + + app := abcimocks.NewApplication(t) + if !testCase.expectPanic { + app.On("PrepareProposal", mock.Anything, mock.Anything).Return(&abci.ResponsePrepareProposal{}, nil) + } + cc := abciclient.NewLocalClient(logger, app) + proxyApp := proxy.New(cc, logger, proxy.NopMetrics()) + err := proxyApp.Start(ctx) + require.NoError(t, err) + + state, stateDB, privVals := makeState(t, 1, int(testCase.height-1)) + stateStore := sm.NewStore(stateDB) + state.ConsensusParams.ABCI.VoteExtensionsEnableHeight = testCase.extensionEnableHeight + mp := &mpmocks.Mempool{} + mp.On("Lock").Return() + mp.On("Unlock").Return() + mp.On("FlushAppConn", mock.Anything).Return(nil) + mp.On("Update", + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything, + mock.Anything).Return(nil) + mp.On("ReapMaxBytesMaxGas", mock.Anything, mock.Anything).Return(types.Txs{}) + + blockExec := sm.NewBlockExecutor( + stateStore, + logger, + proxyApp, + mp, + sm.EmptyEvidencePool{}, + nil, + eventBus, + sm.NopMetrics(), + ) + block := sf.MakeBlock(state, testCase.height, new(types.Commit)) + bps, err := block.MakePartSet(testPartSize) + require.NoError(t, err) + blockID := types.BlockID{Hash: block.Hash(), PartSetHeader: bps.Header()} + pa, _ := state.Validators.GetByIndex(0) + lastCommit, _ := makeValidCommit(ctx, t, testCase.height-1, blockID, state.Validators, privVals) + stripSignatures(lastCommit) + if testCase.expectPanic { + require.Panics(t, func() { + blockExec.CreateProposalBlock(ctx, testCase.height, state, lastCommit, pa) //nolint:errcheck + }) + } else { + _, err = blockExec.CreateProposalBlock(ctx, testCase.height, state, lastCommit, pa) + require.NoError(t, err) + } + }) + } +} + +func stripSignatures(ec *types.ExtendedCommit) { + for i, commitSig := range ec.ExtendedSignatures { + commitSig.Extension = nil + commitSig.ExtensionSignature = nil + ec.ExtendedSignatures[i] = commitSig + } +} + func makeBlockID(hash []byte, partSetSize uint32, partSetHash []byte) types.BlockID { var ( h = make([]byte, crypto.HashSize) diff --git a/internal/state/mocks/block_store.go b/internal/state/mocks/block_store.go index 4eafb1273..58fc640fc 100644 --- a/internal/state/mocks/block_store.go +++ b/internal/state/mocks/block_store.go @@ -209,7 +209,12 @@ func (_m *BlockStore) PruneBlocks(height int64) (uint64, error) { } // SaveBlock provides a mock function with given fields: block, blockParts, seenCommit -func (_m *BlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.ExtendedCommit) { +func (_m *BlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.Commit) { + _m.Called(block, blockParts, seenCommit) +} + +// SaveBlockWithExtendedCommit provides a mock function with given fields: block, blockParts, seenCommit +func (_m *BlockStore) SaveBlockWithExtendedCommit(block *types.Block, blockParts *types.PartSet, seenCommit *types.ExtendedCommit) { _m.Called(block, blockParts, seenCommit) } diff --git a/internal/state/services.go b/internal/state/services.go index 35a91aa11..f86c4e3cf 100644 --- a/internal/state/services.go +++ b/internal/state/services.go @@ -26,7 +26,8 @@ type BlockStore interface { LoadBlockMeta(height int64) *types.BlockMeta LoadBlock(height int64) *types.Block - SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.ExtendedCommit) + SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.Commit) + SaveBlockWithExtendedCommit(block *types.Block, blockParts *types.PartSet, seenCommit *types.ExtendedCommit) PruneBlocks(height int64) (uint64, error) diff --git a/internal/state/store.go b/internal/state/store.go index 2d2e4dc81..d592016f6 100644 --- a/internal/state/store.go +++ b/internal/state/store.go @@ -2,6 +2,7 @@ package state import ( "bytes" + "encoding/binary" "errors" "fmt" @@ -59,6 +60,7 @@ func abciResponsesKey(height int64) []byte { // stateKey should never change after being set in init() var stateKey []byte +var tmpABCIKey []byte func init() { var err error @@ -66,6 +68,12 @@ func init() { if err != nil { panic(err) } + // temporary extra key before consensus param protos are regenerated + // TODO(wbanfield) remove in next PR + tmpABCIKey, err = orderedcode.Append(nil, int64(10000)) + if err != nil { + panic(err) + } } //---------------------- @@ -137,6 +145,12 @@ func (store dbStore) loadState(key []byte) (state State, err error) { if err != nil { return state, err } + buf, err = store.db.Get(tmpABCIKey) + if err != nil { + return state, err + } + h, _ := binary.Varint(buf) + sm.ConsensusParams.ABCI.VoteExtensionsEnableHeight = h return *sm, nil } @@ -181,6 +195,11 @@ func (store dbStore) save(state State, key []byte) error { if err := batch.Set(key, stateBz); err != nil { return err } + bz := make([]byte, 5) + binary.PutVarint(bz, state.ConsensusParams.ABCI.VoteExtensionsEnableHeight) + if err := batch.Set(tmpABCIKey, bz); err != nil { + return err + } return batch.WriteSync() } diff --git a/internal/state/validation_test.go b/internal/state/validation_test.go index b29cfd0f9..0f43db5eb 100644 --- a/internal/state/validation_test.go +++ b/internal/state/validation_test.go @@ -124,7 +124,7 @@ func TestValidateBlockHeader(t *testing.T) { */ state, _, lastExtCommit = makeAndCommitGoodBlock(ctx, t, state, height, lastCommit, state.Validators.GetProposer().Address, blockExec, privVals, nil) - lastCommit = lastExtCommit.StripExtensions() + lastCommit = lastExtCommit.ToCommit() } nextHeight := validationTestsStopHeight @@ -234,7 +234,7 @@ func TestValidateBlockCommit(t *testing.T) { privVals, nil, ) - lastCommit = lastExtCommit.StripExtensions() + lastCommit = lastExtCommit.ToCommit() /* wrongSigsCommit is fine except for the extra bad precommit @@ -384,7 +384,7 @@ func TestValidateBlockEvidence(t *testing.T) { privVals, evidence, ) - lastCommit = lastExtCommit.StripExtensions() + lastCommit = lastExtCommit.ToCommit() } } diff --git a/internal/statesync/reactor_test.go b/internal/statesync/reactor_test.go index 0db516f62..b81c1ac2c 100644 --- a/internal/statesync/reactor_test.go +++ b/internal/statesync/reactor_test.go @@ -859,13 +859,13 @@ func mockLB(ctx context.Context, t *testing.T, height int64, time time.Time, las header.NextValidatorsHash = nextVals.Hash() header.ConsensusHash = types.DefaultConsensusParams().HashConsensusParams() lastBlockID = factory.MakeBlockIDWithHash(header.Hash()) - voteSet := types.NewVoteSet(factory.DefaultTestChainID, height, 0, tmproto.PrecommitType, currentVals) + voteSet := types.NewExtendedVoteSet(factory.DefaultTestChainID, height, 0, tmproto.PrecommitType, currentVals) extCommit, err := factory.MakeExtendedCommit(ctx, lastBlockID, height, 0, voteSet, currentPrivVals, time) require.NoError(t, err) return nextVals, nextPrivVals, &types.LightBlock{ SignedHeader: &types.SignedHeader{ Header: header, - Commit: extCommit.StripExtensions(), + Commit: extCommit.ToCommit(), }, ValidatorSet: currentVals, } diff --git a/internal/store/store.go b/internal/store/store.go index 5617674a2..1ba7e398d 100644 --- a/internal/store/store.go +++ b/internal/store/store.go @@ -2,6 +2,7 @@ package store import ( "bytes" + "errors" "fmt" "strconv" @@ -278,6 +279,9 @@ func (bs *BlockStore) LoadBlockCommit(height int64) *types.Commit { return commit } +// LoadExtendedCommit returns the ExtendedCommit for the given height. +// The extended commit is not guaranteed to contain the same +2/3 precommits data +// as the commit in the block. func (bs *BlockStore) LoadBlockExtendedCommit(height int64) *types.ExtendedCommit { pbec := new(tmproto.ExtendedCommit) bz, err := bs.db.Get(extCommitKey(height)) @@ -466,25 +470,73 @@ func (bs *BlockStore) batchDelete( // If all the nodes restart after committing a block, // we need this to reload the precommits to catch-up nodes to the // most recent height. Otherwise they'd stall at H-1. -func (bs *BlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.ExtendedCommit) { +func (bs *BlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.Commit) { if block == nil { panic("BlockStore can only save a non-nil block") } - batch := bs.db.NewBatch() + if err := bs.saveBlockToBatch(batch, block, blockParts, seenCommit); err != nil { + panic(err) + } + + if err := batch.WriteSync(); err != nil { + panic(err) + } + + if err := batch.Close(); err != nil { + panic(err) + } +} + +// SaveBlockWithExtendedCommit persists the given block, blockParts, and +// seenExtendedCommit to the underlying db. seenExtendedCommit is stored under +// two keys in the database: as the seenCommit and as the ExtendedCommit data for the +// height. This allows the vote extension data to be persisted for all blocks +// that are saved. +func (bs *BlockStore) SaveBlockWithExtendedCommit(block *types.Block, blockParts *types.PartSet, seenExtendedCommit *types.ExtendedCommit) { + if block == nil { + panic("BlockStore can only save a non-nil block") + } + if err := seenExtendedCommit.EnsureExtensions(); err != nil { + panic(fmt.Errorf("saving block with extensions: %w", err)) + } + batch := bs.db.NewBatch() + if err := bs.saveBlockToBatch(batch, block, blockParts, seenExtendedCommit.ToCommit()); err != nil { + panic(err) + } + height := block.Height + + pbec := seenExtendedCommit.ToProto() + extCommitBytes := mustEncode(pbec) + if err := batch.Set(extCommitKey(height), extCommitBytes); err != nil { + panic(err) + } + + if err := batch.WriteSync(); err != nil { + panic(err) + } + + if err := batch.Close(); err != nil { + panic(err) + } +} + +func (bs *BlockStore) saveBlockToBatch(batch dbm.Batch, block *types.Block, blockParts *types.PartSet, seenCommit *types.Commit) error { + if block == nil { + panic("BlockStore can only save a non-nil block") + } height := block.Height hash := block.Hash() if g, w := height, bs.Height()+1; bs.Base() > 0 && g != w { - panic(fmt.Sprintf("BlockStore can only save contiguous blocks. Wanted %v, got %v", w, g)) + return fmt.Errorf("BlockStore can only save contiguous blocks. Wanted %v, got %v", w, g) } if !blockParts.IsComplete() { - panic("BlockStore can only save complete block part sets") + return errors.New("BlockStore can only save complete block part sets") } if height != seenCommit.Height { - panic(fmt.Sprintf("BlockStore cannot save seen commit of a different height (block: %d, commit: %d)", - height, seenCommit.Height)) + return fmt.Errorf("BlockStore cannot save seen commit of a different height (block: %d, commit: %d)", height, seenCommit.Height) } // Save block parts. This must be done before the block meta, since callers @@ -499,44 +551,32 @@ func (bs *BlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, s blockMeta := types.NewBlockMeta(block, blockParts) pbm := blockMeta.ToProto() if pbm == nil { - panic("nil blockmeta") + return errors.New("nil blockmeta") } metaBytes := mustEncode(pbm) if err := batch.Set(blockMetaKey(height), metaBytes); err != nil { - panic(err) + return err } if err := batch.Set(blockHashKey(hash), []byte(fmt.Sprintf("%d", height))); err != nil { - panic(err) + return err } pbc := block.LastCommit.ToProto() blockCommitBytes := mustEncode(pbc) if err := batch.Set(blockCommitKey(height-1), blockCommitBytes); err != nil { - panic(err) + return err } // Save seen commit (seen +2/3 precommits for block) - pbsc := seenCommit.StripExtensions().ToProto() + pbsc := seenCommit.ToProto() seenCommitBytes := mustEncode(pbsc) if err := batch.Set(seenCommitKey(), seenCommitBytes); err != nil { - panic(err) + return err } - pbec := seenCommit.ToProto() - extCommitBytes := mustEncode(pbec) - if err := batch.Set(extCommitKey(height), extCommitBytes); err != nil { - panic(err) - } - - if err := batch.WriteSync(); err != nil { - panic(err) - } - - if err := batch.Close(); err != nil { - panic(err) - } + return nil } func (bs *BlockStore) saveBlockPart(height int64, index int, part *types.Part, batch dbm.Batch) { diff --git a/internal/store/store_test.go b/internal/store/store_test.go index 9df3eed9f..771129cc0 100644 --- a/internal/store/store_test.go +++ b/internal/store/store_test.go @@ -36,6 +36,7 @@ func makeTestExtCommit(height int64, timestamp time.Time) *types.ExtendedCommit Timestamp: timestamp, Signature: []byte("Signature"), }, + ExtensionSignature: []byte("ExtensionSignature"), }} return &types.ExtendedCommit{ Height: height, @@ -89,7 +90,7 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) { part2 := validPartSet.GetPart(1) seenCommit := makeTestExtCommit(block.Header.Height, tmtime.Now()) - bs.SaveBlock(block, validPartSet, seenCommit) + bs.SaveBlockWithExtendedCommit(block, validPartSet, seenCommit) require.EqualValues(t, 1, bs.Base(), "expecting the new height to be changed") require.EqualValues(t, block.Header.Height, bs.Height(), "expecting the new height to be changed") @@ -107,7 +108,7 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) { } // End of setup, test data - commitAtH10 := makeTestExtCommit(10, tmtime.Now()).StripExtensions() + commitAtH10 := makeTestExtCommit(10, tmtime.Now()).ToCommit() tuples := []struct { block *types.Block parts *types.PartSet @@ -140,16 +141,17 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) { ChainID: "block_test", Time: tmtime.Now(), ProposerAddress: tmrand.Bytes(crypto.AddressSize)}, - makeTestExtCommit(5, tmtime.Now()).StripExtensions(), + makeTestExtCommit(5, tmtime.Now()).ToCommit(), ), parts: validPartSet, seenCommit: makeTestExtCommit(5, tmtime.Now()), }, { - block: newBlock(header1, commitAtH10), - parts: incompletePartSet, - wantPanic: "only save complete block", // incomplete parts + block: newBlock(header1, commitAtH10), + parts: incompletePartSet, + wantPanic: "only save complete block", // incomplete parts + seenCommit: makeTestExtCommit(10, tmtime.Now()), }, { @@ -178,7 +180,7 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) { }, { - block: newBlock(header1, commitAtH10), + block: block, parts: validPartSet, seenCommit: seenCommit, @@ -187,7 +189,7 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) { }, { - block: newBlock(header1, commitAtH10), + block: block, parts: validPartSet, seenCommit: seenCommit, @@ -209,7 +211,7 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) { bs, db := newInMemoryBlockStore() // SaveBlock res, err, panicErr := doFn(func() (interface{}, error) { - bs.SaveBlock(tuple.block, tuple.parts, tuple.seenCommit) + bs.SaveBlockWithExtendedCommit(tuple.block, tuple.parts, tuple.seenCommit) if tuple.block == nil { return nil, nil } @@ -279,6 +281,90 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) { } } +// TestSaveBlockWithExtendedCommitPanicOnAbsentExtension tests that saving a +// block with an extended commit panics when the extension data is absent. +func TestSaveBlockWithExtendedCommitPanicOnAbsentExtension(t *testing.T) { + for _, testCase := range []struct { + name string + malleateCommit func(*types.ExtendedCommit) + shouldPanic bool + }{ + { + name: "basic save", + malleateCommit: func(_ *types.ExtendedCommit) {}, + shouldPanic: false, + }, + { + name: "save commit with no extensions", + malleateCommit: func(c *types.ExtendedCommit) { + c.StripExtensions() + }, + shouldPanic: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + state, bs, cleanup, err := makeStateAndBlockStore(t.TempDir()) + require.NoError(t, err) + defer cleanup() + block := factory.MakeBlock(state, bs.Height()+1, new(types.Commit)) + seenCommit := makeTestExtCommit(block.Header.Height, tmtime.Now()) + ps, err := block.MakePartSet(2) + require.NoError(t, err) + testCase.malleateCommit(seenCommit) + if testCase.shouldPanic { + require.Panics(t, func() { + bs.SaveBlockWithExtendedCommit(block, ps, seenCommit) + }) + } else { + bs.SaveBlockWithExtendedCommit(block, ps, seenCommit) + } + }) + } +} + +// TestLoadBlockExtendedCommit tests loading the extended commit for a previously +// saved block. The load method should return nil when only a commit was saved and +// return the extended commit otherwise. +func TestLoadBlockExtendedCommit(t *testing.T) { + for _, testCase := range []struct { + name string + saveExtended bool + expectResult bool + }{ + { + name: "save commit", + saveExtended: false, + expectResult: false, + }, + { + name: "save extended commit", + saveExtended: true, + expectResult: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + state, bs, cleanup, err := makeStateAndBlockStore(t.TempDir()) + require.NoError(t, err) + defer cleanup() + block := factory.MakeBlock(state, bs.Height()+1, new(types.Commit)) + seenCommit := makeTestExtCommit(block.Header.Height, tmtime.Now()) + ps, err := block.MakePartSet(2) + require.NoError(t, err) + if testCase.saveExtended { + bs.SaveBlockWithExtendedCommit(block, ps, seenCommit) + } else { + bs.SaveBlock(block, ps, seenCommit.ToCommit()) + } + res := bs.LoadBlockExtendedCommit(block.Height) + if testCase.expectResult { + require.Equal(t, seenCommit, res) + } else { + require.Nil(t, res) + } + }) + } +} + func TestLoadBaseMeta(t *testing.T) { cfg, err := config.ResetTestRoot(t.TempDir(), "blockchain_reactor_test") require.NoError(t, err) @@ -293,7 +379,7 @@ func TestLoadBaseMeta(t *testing.T) { partSet, err := block.MakePartSet(2) require.NoError(t, err) seenCommit := makeTestExtCommit(h, tmtime.Now()) - bs.SaveBlock(block, partSet, seenCommit) + bs.SaveBlockWithExtendedCommit(block, partSet, seenCommit) } pruned, err := bs.PruneBlocks(4) @@ -371,7 +457,7 @@ func TestPruneBlocks(t *testing.T) { partSet, err := block.MakePartSet(2) require.NoError(t, err) seenCommit := makeTestExtCommit(h, tmtime.Now()) - bs.SaveBlock(block, partSet, seenCommit) + bs.SaveBlockWithExtendedCommit(block, partSet, seenCommit) } assert.EqualValues(t, 1, bs.Base()) @@ -479,7 +565,7 @@ func TestBlockFetchAtHeight(t *testing.T) { partSet, err := block.MakePartSet(2) require.NoError(t, err) seenCommit := makeTestExtCommit(block.Header.Height, tmtime.Now()) - bs.SaveBlock(block, partSet, seenCommit) + bs.SaveBlockWithExtendedCommit(block, partSet, seenCommit) require.Equal(t, bs.Height(), block.Header.Height, "expecting the new height to be changed") blockAtHeight := bs.LoadBlock(bs.Height()) @@ -518,16 +604,16 @@ func TestSeenAndCanonicalCommit(t *testing.T) { // produce a few blocks and check that the correct seen and cannoncial commits // are persisted. for h := int64(3); h <= 5; h++ { - blockCommit := makeTestExtCommit(h-1, tmtime.Now()).StripExtensions() + blockCommit := makeTestExtCommit(h-1, tmtime.Now()).ToCommit() block := factory.MakeBlock(state, h, blockCommit) partSet, err := block.MakePartSet(2) require.NoError(t, err) seenCommit := makeTestExtCommit(h, tmtime.Now()) - store.SaveBlock(block, partSet, seenCommit) + store.SaveBlockWithExtendedCommit(block, partSet, seenCommit) c3 := store.LoadSeenCommit() require.NotNil(t, c3) require.Equal(t, h, c3.Height) - require.Equal(t, seenCommit.StripExtensions().Hash(), c3.Hash()) + require.Equal(t, seenCommit.ToCommit().Hash(), c3.Hash()) c5 := store.LoadBlockCommit(h) require.Nil(t, c5) c6 := store.LoadBlockCommit(h - 1) diff --git a/internal/test/factory/params.go b/internal/test/factory/params.go index dda8e2b3c..c6fa3f9fc 100644 --- a/internal/test/factory/params.go +++ b/internal/test/factory/params.go @@ -18,5 +18,6 @@ func ConsensusParams() *types.ConsensusParams { VoteDelta: 1 * time.Millisecond, BypassCommitTimeout: true, } + c.ABCI.VoteExtensionsEnableHeight = 1 return c } diff --git a/light/provider/http/http.go b/light/provider/http/http.go index cf443e1b5..455c5cbaa 100644 --- a/light/provider/http/http.go +++ b/light/provider/http/http.go @@ -173,8 +173,7 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato attempt := uint16(0) for { res, err := p.client.Validators(ctx, height, &page, &perPage) - switch e := err.(type) { - case nil: // success!! Now we validate the response + if err == nil { if len(res.Validators) == 0 { return nil, provider.ErrBadLightBlock{ Reason: fmt.Errorf("validator set is empty (height: %d, page: %d, per_page: %d)", @@ -187,35 +186,37 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato res.Total, height, page, perPage), } } + } else { + switch e := err.(type) { - case *url.Error: - if e.Timeout() { - // if we have exceeded retry attempts then return a no response error - if attempt == p.maxRetryAttempts { - return nil, p.noResponse() + case *url.Error: + if e.Timeout() { + // if we have exceeded retry attempts then return a no response error + if attempt == p.maxRetryAttempts { + return nil, p.noResponse() + } + attempt++ + // request timed out: we wait and try again with exponential backoff + time.Sleep(backoffTimeout(attempt)) + continue } - attempt++ - // request timed out: we wait and try again with exponential backoff - time.Sleep(backoffTimeout(attempt)) - continue + return nil, provider.ErrBadLightBlock{Reason: e} + + case *rpctypes.RPCError: + // process the rpc error and return the corresponding error to the light client + return nil, p.parseRPCError(e) + + default: + // check if the error stems from the context + if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { + return nil, err + } + + // If we don't know the error then by default we return an unreliable provider error and + // terminate the connection with the peer. + return nil, provider.ErrUnreliableProvider{Reason: e} } - return nil, provider.ErrBadLightBlock{Reason: e} - - case *rpctypes.RPCError: - // process the rpc error and return the corresponding error to the light client - return nil, p.parseRPCError(e) - - default: - // check if the error stems from the context - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { - return nil, err - } - - // If we don't know the error then by default we return an unreliable provider error and - // terminate the connection with the peer. - return nil, provider.ErrUnreliableProvider{Reason: e} } - // update the total and increment the page index so we can fetch the // next page of validators if need be total = res.Total @@ -223,6 +224,7 @@ func (p *http) validatorSet(ctx context.Context, height *int64) (*types.Validato page++ break } + } valSet, err := types.ValidatorSetFromExistingValidators(vals) diff --git a/node/node.go b/node/node.go index c077d6c6c..68509abb4 100644 --- a/node/node.go +++ b/node/node.go @@ -266,7 +266,7 @@ func makeNode( node.evPool = evPool mpReactor, mp := createMempoolReactor(logger, cfg, proxyApp, stateStore, nodeMetrics.mempool, - peerManager.Subscribe, node.router.OpenChannel, peerManager.GetHeight) + peerManager.Subscribe, node.router.OpenChannel) node.rpcEnv.Mempool = mp node.services = append(node.services, mpReactor) diff --git a/node/node_test.go b/node/node_test.go index b1d7a9481..245e39b3c 100644 --- a/node/node_test.go +++ b/node/node_test.go @@ -526,7 +526,7 @@ func TestMaxProposalBlockSize(t *testing.T) { } state.ChainID = maxChainID - voteSet := types.NewVoteSet(state.ChainID, math.MaxInt64-1, math.MaxInt32, tmproto.PrecommitType, state.Validators) + voteSet := types.NewExtendedVoteSet(state.ChainID, math.MaxInt64-1, math.MaxInt32, tmproto.PrecommitType, state.Validators) // add maximum amount of signatures to a single commit for i := 0; i < types.MaxVotesCount; i++ { diff --git a/node/setup.go b/node/setup.go index 7d8bb3165..e0a5335d0 100644 --- a/node/setup.go +++ b/node/setup.go @@ -147,7 +147,6 @@ func createMempoolReactor( memplMetrics *mempool.Metrics, peerEvents p2p.PeerEventSubscriber, chCreator p2p.ChannelCreator, - peerHeight func(types.NodeID) int64, ) (service.Service, mempool.Mempool) { logger = logger.With("module", "mempool") @@ -166,7 +165,6 @@ func createMempoolReactor( mp, chCreator, peerEvents, - peerHeight, ) if cfg.Consensus.WaitForTxs() { diff --git a/rpc/jsonrpc/server/http_server.go b/rpc/jsonrpc/server/http_server.go index 0b715835d..50a37158e 100644 --- a/rpc/jsonrpc/server/http_server.go +++ b/rpc/jsonrpc/server/http_server.go @@ -20,16 +20,27 @@ import ( // Config is a RPC server configuration. type Config struct { - // see netutil.LimitListener + // The maximum number of connections that will be accepted by the listener. + // See https://godoc.org/golang.org/x/net/netutil#LimitListener MaxOpenConnections int - // mirrors http.Server#ReadTimeout + + // Used to set the HTTP server's per-request read timeout. + // See https://godoc.org/net/http#Server.ReadTimeout ReadTimeout time.Duration - // mirrors http.Server#WriteTimeout + + // Used to set the HTTP server's per-request write timeout. Note that this + // affects ALL methods on the server, so it should not be set too low. This + // should be used as a safety valve, not a resource-control timeout. + // + // See https://godoc.org/net/http#Server.WriteTimeout WriteTimeout time.Duration - // MaxBodyBytes controls the maximum number of bytes the - // server will read parsing the request body. + + // Controls the maximum number of bytes the server will read parsing the + // request body. MaxBodyBytes int64 - // mirrors http.Server#MaxHeaderBytes + + // Controls the maximum size of a request header. + // See https://godoc.org/net/http#Server.MaxHeaderBytes MaxHeaderBytes int } @@ -38,9 +49,9 @@ func DefaultConfig() *Config { return &Config{ MaxOpenConnections: 0, // unlimited ReadTimeout: 10 * time.Second, - WriteTimeout: 10 * time.Second, - MaxBodyBytes: int64(1000000), // 1MB - MaxHeaderBytes: 1 << 20, // same as the net/http default + WriteTimeout: 0, // no default timeout + MaxBodyBytes: 1000000, // 1MB + MaxHeaderBytes: 1 << 20, // same as the net/http default } } diff --git a/rpc/jsonrpc/server/rpc_func.go b/rpc/jsonrpc/server/rpc_func.go index 8eba28728..1fff323d7 100644 --- a/rpc/jsonrpc/server/rpc_func.go +++ b/rpc/jsonrpc/server/rpc_func.go @@ -9,11 +9,16 @@ import ( "net/http" "reflect" "strings" + "time" "github.com/tendermint/tendermint/libs/log" rpctypes "github.com/tendermint/tendermint/rpc/jsonrpc/types" ) +// DefaultRPCTimeout is the default context timeout for calls to any RPC method +// that does not override it with a more specific timeout. +const DefaultRPCTimeout = 60 * time.Second + // RegisterRPCFuncs adds a route to mux for each non-websocket function in the // funcMap, and also a root JSON-RPC POST handler. func RegisterRPCFuncs(mux *http.ServeMux, funcMap map[string]*RPCFunc, logger log.Logger) { @@ -32,11 +37,12 @@ func RegisterRPCFuncs(mux *http.ServeMux, funcMap map[string]*RPCFunc, logger lo // RPCFunc contains the introspected type information for a function. type RPCFunc struct { - f reflect.Value // underlying rpc function - param reflect.Type // the parameter struct, or nil - result reflect.Type // the non-error result type, or nil - args []argInfo // names and type information (for URL decoding) - ws bool // websocket only + f reflect.Value // underlying rpc function + param reflect.Type // the parameter struct, or nil + result reflect.Type // the non-error result type, or nil + args []argInfo // names and type information (for URL decoding) + timeout time.Duration // default request timeout, 0 means none + ws bool // websocket only } // argInfo records the name of a field, along with a bit to tell whether the @@ -52,6 +58,12 @@ type argInfo struct { // with the resulting argument value. It reports an error if parameter parsing // fails, otherwise it returns the result from the wrapped function. func (rf *RPCFunc) Call(ctx context.Context, params json.RawMessage) (interface{}, error) { + // If ctx has its own deadline we will respect it; otherwise use rf.timeout. + if _, ok := ctx.Deadline(); !ok && rf.timeout > 0 { + var cancel context.CancelFunc + ctx, cancel = context.WithTimeout(ctx, rf.timeout) + defer cancel() + } args, err := rf.parseParams(ctx, params) if err != nil { return nil, err @@ -74,6 +86,11 @@ func (rf *RPCFunc) Call(ctx context.Context, params json.RawMessage) (interface{ return returns[0].Interface(), nil } +// Timeout updates rf to include a default timeout for calls to rf. This +// timeout is used if one is not already provided on the request context. +// Setting d == 0 means there will be no timeout. Returns rf to allow chaining. +func (rf *RPCFunc) Timeout(d time.Duration) *RPCFunc { rf.timeout = d; return rf } + // parseParams parses the parameters of a JSON-RPC request and returns the // corresponding argument values. On success, the first argument value will be // the value of ctx. @@ -129,7 +146,9 @@ func (rf *RPCFunc) adjustParams(data []byte) (json.RawMessage, error) { // func(context.Context, *T) (R, error) // // for an arbitrary struct type T and type R. NewRPCFunc will panic if f does -// not have one of these forms. +// not have one of these forms. A newly-constructed RPCFunc has a default +// timeout of DefaultRPCTimeout; use the Timeout method to adjust this as +// needed. func NewRPCFunc(f interface{}) *RPCFunc { rf, err := newRPCFunc(f) if err != nil { @@ -215,10 +234,11 @@ func newRPCFunc(f interface{}) (*RPCFunc, error) { } return &RPCFunc{ - f: fv, - param: ptype, - result: rtype, - args: args, + f: fv, + param: ptype, + result: rtype, + args: args, + timeout: DefaultRPCTimeout, // until overridden }, nil } diff --git a/spec/abci++/README.md b/spec/abci++/README.md index 38feba9d7..a22babfee 100644 --- a/spec/abci++/README.md +++ b/spec/abci++/README.md @@ -25,19 +25,15 @@ This allows Tendermint to run with applications written in many programming lang This specification is split as follows: -- [Overview and basic concepts](./abci++_basic_concepts_002_draft.md) - interface's overview and concepts needed to understand other parts of this specification. +- [Overview and basic concepts](./abci++_basic_concepts_002_draft.md) - interface's overview and concepts + needed to understand other parts of this specification. - [Methods](./abci++_methods_002_draft.md) - complete details on all ABCI++ methods and message types. - [Requirements for the Application](./abci++_app_requirements_002_draft.md) - formal requirements - on the Application's logic to ensure liveness of Tendermint. These requirements define what - Tendermint expects from the Application. + on the Application's logic to ensure Tendermint properties such as liveness. These requirements define what + Tendermint expects from the Application; second part on managing ABCI application state and related topics. - [Tendermint's expected behavior](./abci++_tmint_expected_behavior_002_draft.md) - specification of how the different ABCI++ methods may be called by Tendermint. This explains what the Application is to expect from Tendermint. - ->**TODO** Re-read these and remove redundant info - -- [Applications](../abci/apps.md) - how to manage ABCI application state and other - details about building ABCI applications - [Client and Server](../abci/client-server.md) - for those looking to implement their own ABCI application servers diff --git a/spec/abci++/abci++_app_requirements_002_draft.md b/spec/abci++/abci++_app_requirements_002_draft.md index 68014a536..ff9df2c56 100644 --- a/spec/abci++/abci++_app_requirements_002_draft.md +++ b/spec/abci++/abci++_app_requirements_002_draft.md @@ -5,6 +5,8 @@ title: Application Requirements # Application Requirements +## Formal Requirements + This section specifies what Tendermint expects from the Application. It is structured as a set of formal requirements that can be used for testing and verification of the Application's logic. @@ -177,3 +179,846 @@ Likewise, `ExtendVote` can also be non-deterministic: but may also depend on other values or operations. * *wrp = wrq ⇏ erp = erq* + +## Managing the Application state and related topics + +### Connection State + +Tendermint maintains four concurrent ABCI++ connections, namely +[Consensus Connection](#consensus-connection), +[Mempool Connection](#mempool-connection), +[Info/Query Connection](#infoquery-connection), and +[Snapshot Connection](#snapshot-connection). +It is common for an application to maintain a distinct copy of +the state for each connection, which are synchronized upon `Commit` calls. + +#### Concurrency + +In principle, each of the four ABCI++ connections operates concurrently with one +another. This means applications need to ensure access to state is +thread safe. Up to v0.35.x, both the +[default in-process ABCI client](https://github.com/tendermint/tendermint/blob/v0.35.x/abci/client/local_client.go#L18) +and the +[default Go ABCI server](https://github.com/tendermint/tendermint/blob/v0.35.x/abci/server/socket_server.go#L32) +used a global lock to guard the handling of events across all connections, so they were not +concurrent at all. This meant whether your app was compiled in-process with +Tendermint using the `NewLocalClient`, or run out-of-process using the `SocketServer`, +ABCI messages from all connections were received in sequence, one at a +time. +This is no longer the case starting from v0.36.0: the global locks have been removed and it is +up to the Application to synchronize access to its state when handling +ABCI++ methods on all connections. +Nevertheless, as all ABCI calls are now synchronous, ABCI messages using the same connection are +still received in sequence. + +#### FinalizeBlock + +When the consensus algorithm decides on a block, Tendermint uses `FinalizeBlock` to send the +decided block's data to the Application, which uses it to transition its state. + +The Application must remember the latest height from which it +has run a successful `Commit` so that it can tell Tendermint where to +pick up from when it recovers from a crash. See information on the Handshake +[here](#crash-recovery). + +#### Commit + +The Application should persist its state during `Commit`, before returning from it. + +Before invoking `Commit`, Tendermint locks the mempool and flushes the mempool connection. This ensures that +no new messages +will be received on the mempool connection during this processing step, providing an opportunity to safely +update all four +connection states to the latest committed state at the same time. + +When `Commit` returns, Tendermint unlocks the mempool. + +WARNING: if the ABCI app logic processing the `Commit` message sends a +`/broadcast_tx_sync` or `/broadcast_tx` and waits for the response +before proceeding, it will deadlock. Executing `broadcast_tx` calls +involves acquiring the mempool lock that Tendermint holds during the `Commit` call. +Synchronous mempool-related calls must be avoided as part of the sequential logic of the +`Commit` function. + +#### Candidate States + +Tendermint calls `PrepareProposal` when it is about to send a proposed block to the network. +Likewise, Tendermint calls `ProcessProposal` upon reception of a proposed block from the +network. In both cases, the proposed block's data +is disclosed to the Application, in the same conditions as is done in `FinalizeBlock`. +The block data disclosed the to Application by these three methods are the following: + +* the transaction list +* the `LastCommit` referring to the previous block +* the block header's hash (except in `PrepareProposal`, where it is not known yet) +* list of validators that misbehaved +* the block's timestamp +* `NextValidatorsHash` +* Proposer address + +The Application may decide to *immediately* execute the given block (i.e., upon `PrepareProposal` +or `ProcessProposal`). There are two main reasons why the Application may want to do this: + +* *Avoiding invalid transactions in blocks*. + In order to be sure that the block does not contain *any* invalid transaction, there may be + no way other than fully executing the transactions in the block as though it was the *decided* + block. +* *Quick `FinalizeBlock` execution*. + Upon reception of the decided block via `FinalizeBlock`, if that same block was executed + upon `PrepareProposal` or `ProcessProposal` and the resulting state was kept in memory, the + Application can simply apply that state (faster) to the main state, rather than reexecuting + the decided block (slower). + +`PrepareProposal`/`ProcessProposal` can be called many times for a given height. Moreover, +it is not possible to accurately predict which of the blocks proposed in a height will be decided, +being delivered to the Application in that height's `FinalizeBlock`. +Therefore, the state resulting from executing a proposed block, denoted a *candidate state*, should +be kept in memory as a possible final state for that height. When `FinalizeBlock` is called, the Application should +check if the decided block corresponds to one of its candidate states; if so, it will apply it as +its *ExecuteTxState* (see [Consensus Connection](#consensus-connection) below), +which will be persisted during the upcoming `Commit` call. + +Under adverse conditions (e.g., network instability), Tendermint might take many rounds. +In this case, potentially many proposed blocks will be disclosed to the Application for a given height. +By the nature of Tendermint's consensus algorithm, the number of proposed blocks received by the Application +for a particular height cannot be bound, so Application developers must act with care and use mechanisms +to bound memory usage. As a general rule, the Application should be ready to discard candidate states +before `FinalizeBlock`, even if one of them might end up corresponding to the +decided block and thus have to be reexecuted upon `FinalizeBlock`. + +### States and ABCI++ Connections + +#### Consensus Connection + +The Consensus Connection should maintain an *ExecuteTxState* — the working state +for block execution. It should be updated by the call to `FinalizeBlock` +during block execution and committed to disk as the "latest +committed state" during `Commit`. Execution of a proposed block (via `PrepareProposal`/`ProcessProposal`) +**must not** update the *ExecuteTxState*, but rather be kept as a separate candidate state until `FinalizeBlock` +confirms which of the candidate states (if any) can be used to update *ExecuteTxState*. + +#### Mempool Connection + +The mempool Connection maintains *CheckTxState*. Tendermint sequentially processes an incoming +transaction (via RPC from client or P2P from the gossip layer) against *CheckTxState*. +If the processing does not return any error, the transaction is accepted into the mempool +and Tendermint starts gossipping it. +*CheckTxState* should be reset to the latest committed state +at the end of every `Commit`. + +During the execution of a consensus instance, the *CheckTxState* may be updated concurrently with the +*ExecuteTxState*, as messages may be sent concurrently on the Consensus and Mempool connections. +At the end of the consensus instance, as described above, Tendermint locks the mempool and flushes +the mempool connection before calling `Commit`. This ensures that all pending `CheckTx` calls are +responded to and no new ones can begin. + +After the `Commit` call returns, while still holding the mempool lock, `CheckTx` is run again on all +transactions that remain in the node's local mempool after filtering those included in the block. +Parameter `Type` in `RequestCheckTx` +indicates whether an incoming transaction is new (`CheckTxType_New`), or a +recheck (`CheckTxType_Recheck`). + +Finally, after re-checking transactions in the mempool, Tendermint will unlock +the mempool connection. New transactions are once again able to be processed through `CheckTx`. + +Note that `CheckTx` is just a weak filter to keep invalid transactions out of the mempool and, +utimately, ouf of the blockchain. +Since the transaction cannot be guaranteed to be checked against the exact same state as it +will be executed as part of a (potential) decided block, `CheckTx` shouldn't check *everything* +that affects the transaction's validity, in particular those checks whose validity may depend on +transaction ordering. `CheckTx` is weak because a Byzantine node need not care about `CheckTx`; +it can propose a block full of invalid transactions if it wants. The mechanism ABCI++ has +in place for dealing with such behavior is `ProcessProposal`. + +##### Replay Protection + +It is possible for old transactions to be sent again to the Application. This is typically +undesirable for all transactions, except for a generally small subset of them which are idempotent. + +The mempool has a mechanism to prevent duplicated transactions from being processed. +This mechanism is nevertheless best-effort (currently based on the indexer) +and does not provide any guarantee of non duplication. +It is thus up to the Application to implement an application-specific +replay protection mechanism with strong guarantees as part of the logic in `CheckTx`. + +#### Info/Query Connection + +The Info (or Query) Connection should maintain a `QueryState`. This connection has two +purposes: 1) having the application answer the queries Tenderissued receives from users +(see section [Query](#query)), +and 2) synchronizing Tendermint and the Application at start up time (see +[Crash Recovery](#crash-recovery)) +or after state sync (see [State Sync](#state-sync)). + +`QueryState` is a read-only copy of *ExecuteTxState* as it was after the last +`Commit`, i.e. +after the full block has been processed and the state committed to disk. + +#### Snapshot Connection + +The Snapshot Connection is used to serve state sync snapshots for other nodes +and/or restore state sync snapshots to a local node being bootstrapped. +Snapshop management is optional: an Application may choose not to implement it. + +For more information, see Section [State Sync](#state-sync). + +### Transaction Results + +The Application is expected to return a list of +[`ExecTxResult`](./abci%2B%2B_methods_002_draft.md#exectxresult) in +[`ResponseFinalizeBlock`](./abci%2B%2B_methods_002_draft.md#finalizeblock). The list of transaction +results must respect the same order as the list of transactions delivered via +[`RequestFinalizeBlock`](./abci%2B%2B_methods_002_draft.md#finalizeblock). +This section discusses the fields inside this structure, along with the fields in +[`ResponseCheckTx`](./abci%2B%2B_methods_002_draft.md#checktx), +whose semantics are similar. + +The `Info` and `Log` fields are +non-deterministic values for debugging/convenience purposes. Tendermint logs them but they +are otherwise ignored. + +#### Gas + +Ethereum introduced the notion of *gas* as an abstract representation of the +cost of the resources consumed by nodes when processing a transaction. Every operation in the +Ethereum Virtual Machine uses some amount of gas. +Gas has a market-variable price based on which miners can accept or reject to execute a +particular operation. + +Users propose a maximum amount of gas for their transaction; if the transaction uses less, they get +the difference credited back. Tendermint adopts a similar abstraction, +though uses it only optionally and weakly, allowing applications to define +their own sense of the cost of execution. + +In Tendermint, the [ConsensusParams.Block.MaxGas](#consensus-parameters) limits the amount of +total gas that can be used by all transactions in a block. +The default value is `-1`, which means the block gas limit is not enforced, or that the concept of +gas is meaningless. + +Responses contain a `GasWanted` and `GasUsed` field. The former is the maximum +amount of gas the sender of a transaction is willing to use, and the latter is how much it actually +used. Applications should enforce that `GasUsed <= GasWanted` — i.e. transaction execution +or validation should fail before it can use more resources than it requested. + +When `MaxGas > -1`, Tendermint enforces the following rules: + +* `GasWanted <= MaxGas` for every transaction in the mempool +* `(sum of GasWanted in a block) <= MaxGas` when proposing a block + +If `MaxGas == -1`, no rules about gas are enforced. + +In v0.35.x and earlier versions, Tendermint does not enforce anything about Gas in consensus, +only in the mempool. +This means it does not guarantee that committed blocks satisfy these rules. +It is the application's responsibility to return non-zero response codes when gas limits are exceeded +when executing the transactions of a block. +Since the introduction of `PrepareProposal` and `ProcessProposal` in v.0.36.x, it is now possible +for the Application to enforce that all blocks proposed (and voted for) in consensus — and thus all +blocks decided — respect the `MaxGas` limits described above. + +Since the Application should enforce that `GasUsed <= GasWanted` when executing a transaction, and +it can use `PrepareProposal` and `ProcessProposal` to enforce that `(sum of GasWanted in a block) <= MaxGas` +in all proposed or prevoted blocks, +we have: + +* `(sum of GasUsed in a block) <= MaxGas` for every block + +The `GasUsed` field is ignored by Tendermint. + +#### Specifics of `ResponseCheckTx` + +If `Code != 0`, it will be rejected from the mempool and hence +not broadcasted to other peers and not included in a proposal block. + +`Data` contains the result of the `CheckTx` transaction execution, if any. It does not need to be +deterministic since, given a transaction, nodes' Applications +might have a different *CheckTxState* values when they receive it and check their validity +via `CheckTx`. +Tendermint ignores this value in `ResponseCheckTx`. + +`Events` include any events for the execution, though since the transaction has not +been committed yet, they are effectively ignored by Tendermint. + +From v0.35.x on, there is a `Priority` field in `ResponseCheckTx` that can be +used to explicitly prioritize transactions in the mempool for inclusion in a block +proposal. + +#### Specifics of `ExecTxResult` + +`FinalizeBlock` is the workhorse of the blockchain. Tendermint delivers the decided block, +including the list of all its transactions synchronously to the Application. +The block delivered (and thus the transaction order) is the same at all correct nodes as guaranteed +by the Agreement property of Tendermint consensus. + +In same block execution mode, field `LastResultsHash` in the block header refers to the results +of all transactions stored in that block. Therefore, +`PrepareProposal` must return `ExecTxResult` so that it can +be used to build the block to be proposed in the current height. + +The `Data` field in `ExecTxResult` contains an array of bytes with the transaction result. +It must be deterministic (i.e., the same value must be returned at all nodes), but it can contain arbitrary +data. Likewise, the value of `Code` must be deterministic. +If `Code != 0`, the transaction will be marked invalid, +though it is still included in the block. Invalid transaction are not indexed, as they are +considered analogous to those that failed `CheckTx`. + +Both the `Code` and `Data` are included in a structure that is hashed into the +`LastResultsHash` of the block header in the next height (next block execution mode), or the +header of the block to propose in the current height (same block execution mode, `ExecTxResult` as +part of `PrepareProposal`). + +`Events` include any events for the execution, which Tendermint will use to index +the transaction by. This allows transactions to be queried according to what +events took place during their execution. + +### Updating the Validator Set + +The application may set the validator set during +[`InitChain`](./abci%2B%2B_methods_002_draft.md#initchain), and may update it during +[`FinalizeBlock`](./abci%2B%2B_methods_002_draft.md#finalizeblock) +(next block execution mode) or +[`PrepareProposal`](./abci%2B%2B_methods_002_draft.md#prepareproposal)/[`ProcessProposal`](./abci%2B%2B_methods_002_draft.md#processproposal) +(same block execution mode). In all cases, a structure of type +[`ValidatorUpdate`](./abci%2B%2B_methods_002_draft.md#validatorupdate) is returned. + +The `InitChain` method, used to initialize the Application, can return a list of validators. +If the list is empty, Tendermint will use the validators loaded from the genesis +file. +If the list returned by `InitChain` is not empty, Tendermint will use its contents as the validator set. +This way the application can set the initial validator set for the +blockchain. + +Applications must ensure that a single set of validator updates does not contain duplicates, i.e. +a given public key can only appear once within a given update. If an update includes +duplicates, the block execution will fail irrecoverably. + +Structure `ValidatorUpdate` contains a public key, which is used to identify the validator: +The public key currently supports three types: + +* `ed25519` +* `secp256k1` +* `sr25519` + +Structure `ValidatorUpdate` also contains an `ìnt64` field denoting the validator's new power. +Applications must ensure that +`ValidatorUpdate` structures abide by the following rules: + +* power must be non-negative +* if power is set to 0, the validator must be in the validator set; it will be removed from the set +* if power is greater than 0: + * if the validator is not in the validator set, it will be added to the + set with the given power + * if the validator is in the validator set, its power will be adjusted to the given power +* the total power of the new validator set must not exceed `MaxTotalVotingPower`, where + `MaxTotalVotingPower = MaxInt64 / 8` + +Note the updates returned after processing the block at height `H` will only take effect +at block `H+2` (see Section [Methods](./abci%2B%2B_methods_002_draft.md)). + +### Consensus Parameters + +`ConsensusParams` are global parameters that apply to all validators in a blockchain. +They enforce certain limits in the blockchain, like the maximum size +of blocks, amount of gas used in a block, and the maximum acceptable age of +evidence. They can be set in +[`InitChain`](./abci%2B%2B_methods_002_draft.md#initchain), and updated in +[`FinalizeBlock`](./abci%2B%2B_methods_002_draft.md#finalizeblock) +(next block execution mode) or +[`PrepareProposal`](./abci%2B%2B_methods_002_draft.md#prepareproposal)/[`ProcessProposal`](./abci%2B%2B_methods_002_draft.md#processproposal) +(same block execution model). +These parameters are deterministically set and/or updated by the Application, so +all full nodes have the same value at a given height. + +#### List of Parameters + +These are the current consensus parameters (as of v0.36.x): + +1. [BlockParams.MaxBytes](#blockparamsmaxbytes) +2. [BlockParams.MaxGas](#blockparamsmaxgas) +3. [EvidenceParams.MaxAgeDuration](#evidenceparamsmaxageduration) +4. [EvidenceParams.MaxAgeNumBlocks](#evidenceparamsmaxagenumblocks) +5. [EvidenceParams.MaxBytes](#evidenceparamsmaxbytes) +6. [SynchronyParams.MessageDelay](#synchronyparamsmessagedelay) +7. [SynchronyParams.Precision](#synchronyparamsprecision) +8. [TimeoutParams.Propose](#timeoutparamspropose) +9. [TimeoutParams.ProposeDelta](#timeoutparamsproposedelta) +10. [TimeoutParams.Vote](#timeoutparamsvote) +11. [TimeoutParams.VoteDelta](#timeoutparamsvotedelta) +12. [TimeoutParams.Commit](#timeoutparamscommit) +13. [TimeoutParams.BypassCommitTimeout](#timeoutparamsbypasscommittimeout) + +##### BlockParams.MaxBytes + +The maximum size of a complete Protobuf encoded block. +This is enforced by Tendermint consensus. + +This implies a maximum transaction size that is this `MaxBytes`, less the expected size of +the header, the validator set, and any included evidence in the block. + +Must have `0 < MaxBytes < 100 MB`. + +##### BlockParams.MaxGas + +The maximum of the sum of `GasWanted` that will be allowed in a proposed block. +This is *not* enforced by Tendermint consensus. +It is left to the Application to enforce (ie. if transactions are included past the +limit, they should return non-zero codes). It is used by Tendermint to limit the +transactions included in a proposed block. + +Must have `MaxGas >= -1`. +If `MaxGas == -1`, no limit is enforced. + +##### EvidenceParams.MaxAgeDuration + +This is the maximum age of evidence in time units. +This is enforced by Tendermint consensus. + +If a block includes evidence older than this (AND the evidence was created more +than `MaxAgeNumBlocks` ago), the block will be rejected (validators won't vote +for it). + +Must have `MaxAgeDuration > 0`. + +##### EvidenceParams.MaxAgeNumBlocks + +This is the maximum age of evidence in blocks. +This is enforced by Tendermint consensus. + +If a block includes evidence older than this (AND the evidence was created more +than `MaxAgeDuration` ago), the block will be rejected (validators won't vote +for it). + +Must have `MaxAgeNumBlocks > 0`. + +##### EvidenceParams.MaxBytes + +This is the maximum size of total evidence in bytes that can be committed to a +single block. It should fall comfortably under the max block bytes. + +Its value must not exceed the size of +a block minus its overhead ( ~ `BlockParams.MaxBytes`). + +Must have `MaxBytes > 0`. + +##### SynchronyParams.MessageDelay + +This sets a bound on how long a proposal message may take to reach all +validators on a network and still be considered valid. + +This parameter is part of the +[proposer-based timestamps](../consensus/proposer-based-timestamp) +(PBTS) algorithm. + +##### SynchronyParams.Precision + +This sets a bound on how skewed a proposer's clock may be from any validator +on the network while still producing valid proposals. + +This parameter is part of the +[proposer-based timestamps](../consensus/proposer-based-timestamp) +(PBTS) algorithm. + +##### TimeoutParams.Propose + +Timeout in ms of the propose step of the Tendermint consensus algorithm. +This value is the initial timeout at every height (round 0). + +The value in subsequent rounds is modified by parameter `ProposeDelta`. +When a new height is started, the `Propose` timeout value is reset to this +parameter. + +If a node waiting for a proposal message does not receive one matching its +current height and round before this timeout, the node will issue a +`nil` prevote for the round and advance to the next step. + +##### TimeoutParams.ProposeDelta + +Increment in ms to be added to the `Propose` timeout every time the Tendermint +consensus algorithm advances one round in a given height. + +When a new height is started, the `Propose` timeout value is reset. + +##### TimeoutParams.Vote + +Timeout in ms of the prevote and precommit steps of the Tendermint consensus +algorithm. +This value is the initial timeout at every height (round 0). + +The value in subsequent rounds is modified by parameter `VoteDelta`. +When a new height is started, the `Vote` timeout value is reset to this +parameter. + +The `Vote` timeout does not begin until a quorum of votes has been received. +Once a quorum of votes has been seen and this timeout elapses, Tendermint will +procced to the next step of the consensus algorithm. If Tendermint receives +all of the remaining votes before the end of the timeout, it will proceed +to the next step immediately. + +##### TimeoutParams.VoteDelta + +Increment in ms to be added to the `Vote` timeout every time the Tendermint +consensus algorithm advances one round in a given height. + +When a new height is started, the `Vote` timeout value is reset. + +##### TimeoutParams.Commit + +This configures how long Tendermint will wait after receiving a quorum of +precommits before beginning consensus for the next height. This can be +used to allow slow precommits to arrive for inclusion in the next height +before progressing. + +##### TimeoutParams.BypassCommitTimeout + +This configures the node to proceed immediately to the next height once the +node has received all precommits for a block, forgoing the remaining commit timeout. +Setting this parameter to `false` (the default) causes Tendermint to wait +for the full commit timeout configured in `TimeoutParams.Commit`. + +#### Updating Consensus Parameters + +The application may set the `ConsensusParams` during +[`InitChain`](./abci%2B%2B_methods_002_draft.md#initchain), +and update them during +[`FinalizeBlock`](./abci%2B%2B_methods_002_draft.md#finalizeblock) +(next block execution mode) or +[`PrepareProposal`](./abci%2B%2B_methods_002_draft.md#prepareproposal)/[`ProcessProposal`](./abci%2B%2B_methods_002_draft.md#processproposal) +(same block execution mode). +If the `ConsensusParams` is empty, it will be ignored. Each field +that is not empty will be applied in full. For instance, if updating the +`Block.MaxBytes`, applications must also set the other `Block` fields (like +`Block.MaxGas`), even if they are unchanged, as they will otherwise cause the +value to be updated to the default. + +##### `InitChain` + +`ResponseInitChain` includes a `ConsensusParams` parameter. +If `ConsensusParams` is `nil`, Tendermint will use the params loaded in the genesis +file. If `ConsensusParams` is not `nil`, Tendermint will use it. +This way the application can determine the initial consensus parameters for the +blockchain. + +##### `FinalizeBlock`, `PrepareProposal`/`ProcessProposal` + +In next block execution mode, `ResponseFinalizeBlock` accepts a `ConsensusParams` parameter. +If `ConsensusParams` is `nil`, Tendermint will do nothing. +If `ConsensusParams` is not `nil`, Tendermint will use it. +This way the application can update the consensus parameters over time. + +Likewise, in same block execution mode, `PrepareProposal` and `ProcessProposal` include +a `ConsensusParams` parameter. `PrepareProposal` may return a `ConsensusParams` to update +the consensus parameters in the block that is about to be proposed. If it returns `nil` +the consensus parameters will not be updated. `ProcessProposal` also accepts a +`ConsensusParams` parameter, which Tendermint will use it to calculate the corresponding +hashes and sanity-check them against those of the block that triggered `ProcessProposal` +at the first place. + +Note the updates returned in block `H` will take effect right away for block +`H+1` (both in next block and same block execution mode). + +### `Query` + +`Query` is a generic method with lots of flexibility to enable diverse sets +of queries on application state. Tendermint makes use of `Query` to filter new peers +based on ID and IP, and exposes `Query` to the user over RPC. + +Note that calls to `Query` are not replicated across nodes, but rather query the +local node's state - hence they may return stale reads. For reads that require +consensus, use a transaction. + +The most important use of `Query` is to return Merkle proofs of the application state at some height +that can be used for efficient application-specific light-clients. + +Note Tendermint has technically no requirements from the `Query` +message for normal operation - that is, the ABCI app developer need not implement +Query functionality if they do not wish to. + +#### Query Proofs + +The Tendermint block header includes a number of hashes, each providing an +anchor for some type of proof about the blockchain. The `ValidatorsHash` enables +quick verification of the validator set, the `DataHash` gives quick +verification of the transactions included in the block. + +The `AppHash` is unique in that it is application specific, and allows for +application-specific Merkle proofs about the state of the application. +While some applications keep all relevant state in the transactions themselves +(like Bitcoin and its UTXOs), others maintain a separated state that is +computed deterministically *from* transactions, but is not contained directly in +the transactions themselves (like Ethereum contracts and accounts). +For such applications, the `AppHash` provides a much more efficient way to verify light-client proofs. + +ABCI applications can take advantage of more efficient light-client proofs for +their state as follows: + +* in next block executon mode, return the Merkle root of the deterministic application state in + `ResponseCommit.Data`. This Merkle root will be included as the `AppHash` in the next block. +* in same block execution mode, return the Merkle root of the deterministic application state + in `ResponsePrepareProposal.AppHash`. This Merkle root will be included as the `AppHash` in + the block that is about to be proposed. +* return efficient Merkle proofs about that application state in `ResponseQuery.Proof` + that can be verified using the `AppHash` of the corresponding block. + +For instance, this allows an application's light-client to verify proofs of +absence in the application state, something which is much less efficient to do using the block hash. + +Some applications (eg. Ethereum, Cosmos-SDK) have multiple "levels" of Merkle trees, +where the leaves of one tree are the root hashes of others. To support this, and +the general variability in Merkle proofs, the `ResponseQuery.Proof` has some minimal structure: + +```protobuf +message ProofOps { + repeated ProofOp ops = 1 +} + +message ProofOp { + string type = 1; + bytes key = 2; + bytes data = 3; +} +``` + +Each `ProofOp` contains a proof for a single key in a single Merkle tree, of the specified `type`. +This allows ABCI to support many different kinds of Merkle trees, encoding +formats, and proofs (eg. of presence and absence) just by varying the `type`. +The `data` contains the actual encoded proof, encoded according to the `type`. +When verifying the full proof, the root hash for one ProofOp is the value being +verified for the next ProofOp in the list. The root hash of the final ProofOp in +the list should match the `AppHash` being verified against. + +#### Peer Filtering + +When Tendermint connects to a peer, it sends two queries to the ABCI application +using the following paths, with no additional data: + +* `/p2p/filter/addr/`, where `` denote the IP address and + the port of the connection +* `p2p/filter/id/`, where `` is the peer node ID (ie. the + pubkey.Address() for the peer's PubKey) + +If either of these queries return a non-zero ABCI code, Tendermint will refuse +to connect to the peer. + +#### Paths + +Queries are directed at paths, and may optionally include additional data. + +The expectation is for there to be some number of high level paths +differentiating concerns, like `/p2p`, `/store`, and `/app`. Currently, +Tendermint only uses `/p2p`, for filtering peers. For more advanced use, see the +implementation of +[Query in the Cosmos-SDK](https://github.com/cosmos/cosmos-sdk/blob/v0.23.1/baseapp/baseapp.go#L333). + +### Crash Recovery + +On startup, Tendermint calls the `Info` method on the Info Connection to get the latest +committed state of the app. The app MUST return information consistent with the +last block it succesfully completed Commit for. + +If the app succesfully committed block H, then `last_block_height = H` and `last_block_app_hash = `. If the app +failed during the Commit of block H, then `last_block_height = H-1` and +`last_block_app_hash = `. + +We now distinguish three heights, and describe how Tendermint syncs itself with +the app. + +```md +storeBlockHeight = height of the last block Tendermint saw a commit for +stateBlockHeight = height of the last block for which Tendermint completed all + block processing and saved all ABCI results to disk +appBlockHeight = height of the last block for which ABCI app succesfully + completed Commit + +``` + +Note we always have `storeBlockHeight >= stateBlockHeight` and `storeBlockHeight >= appBlockHeight` +Note also Tendermint never calls Commit on an ABCI app twice for the same height. + +The procedure is as follows. + +First, some simple start conditions: + +If `appBlockHeight == 0`, then call InitChain. + +If `storeBlockHeight == 0`, we're done. + +Now, some sanity checks: + +If `storeBlockHeight < appBlockHeight`, error +If `storeBlockHeight < stateBlockHeight`, panic +If `storeBlockHeight > stateBlockHeight+1`, panic + +Now, the meat: + +If `storeBlockHeight == stateBlockHeight && appBlockHeight < storeBlockHeight`, +replay all blocks in full from `appBlockHeight` to `storeBlockHeight`. +This happens if we completed processing the block, but the app forgot its height. + +If `storeBlockHeight == stateBlockHeight && appBlockHeight == storeBlockHeight`, we're done. +This happens if we crashed at an opportune spot. + +If `storeBlockHeight == stateBlockHeight+1` +This happens if we started processing the block but didn't finish. + +If `appBlockHeight < stateBlockHeight` + replay all blocks in full from `appBlockHeight` to `storeBlockHeight-1`, + and replay the block at `storeBlockHeight` using the WAL. +This happens if the app forgot the last block it committed. + +If `appBlockHeight == stateBlockHeight`, + replay the last block (storeBlockHeight) in full. +This happens if we crashed before the app finished Commit + +If `appBlockHeight == storeBlockHeight` + update the state using the saved ABCI responses but dont run the block against the real app. +This happens if we crashed after the app finished Commit but before Tendermint saved the state. + +### State Sync + +A new node joining the network can simply join consensus at the genesis height and replay all +historical blocks until it is caught up. However, for large chains this can take a significant +amount of time, often on the order of days or weeks. + +State sync is an alternative mechanism for bootstrapping a new node, where it fetches a snapshot +of the state machine at a given height and restores it. Depending on the application, this can +be several orders of magnitude faster than replaying blocks. + +Note that state sync does not currently backfill historical blocks, so the node will have a +truncated block history - users are advised to consider the broader network implications of this in +terms of block availability and auditability. This functionality may be added in the future. + +For details on the specific ABCI calls and types, see the +[methods](abci%2B%2B_methods_002_draft.md) section. + +#### Taking Snapshots + +Applications that want to support state syncing must take state snapshots at regular intervals. How +this is accomplished is entirely up to the application. A snapshot consists of some metadata and +a set of binary chunks in an arbitrary format: + +* `Height (uint64)`: The height at which the snapshot is taken. It must be taken after the given + height has been committed, and must not contain data from any later heights. + +* `Format (uint32)`: An arbitrary snapshot format identifier. This can be used to version snapshot + formats, e.g. to switch from Protobuf to MessagePack for serialization. The application can use + this when restoring to choose whether to accept or reject a snapshot. + +* `Chunks (uint32)`: The number of chunks in the snapshot. Each chunk contains arbitrary binary + data, and should be less than 16 MB; 10 MB is a good starting point. + +* `Hash ([]byte)`: An arbitrary hash of the snapshot. This is used to check whether a snapshot is + the same across nodes when downloading chunks. + +* `Metadata ([]byte)`: Arbitrary snapshot metadata, e.g. chunk hashes for verification or any other + necessary info. + +For a snapshot to be considered the same across nodes, all of these fields must be identical. When +sent across the network, snapshot metadata messages are limited to 4 MB. + +When a new node is running state sync and discovering snapshots, Tendermint will query an existing +application via the ABCI `ListSnapshots` method to discover available snapshots, and load binary +snapshot chunks via `LoadSnapshotChunk`. The application is free to choose how to implement this +and which formats to use, but must provide the following guarantees: + +* **Consistent:** A snapshot must be taken at a single isolated height, unaffected by + concurrent writes. This can be accomplished by using a data store that supports ACID + transactions with snapshot isolation. + +* **Asynchronous:** Taking a snapshot can be time-consuming, so it must not halt chain progress, + for example by running in a separate thread. + +* **Deterministic:** A snapshot taken at the same height in the same format must be identical + (at the byte level) across nodes, including all metadata. This ensures good availability of + chunks, and that they fit together across nodes. + +A very basic approach might be to use a datastore with MVCC transactions (such as RocksDB), +start a transaction immediately after block commit, and spawn a new thread which is passed the +transaction handle. This thread can then export all data items, serialize them using e.g. +Protobuf, hash the byte stream, split it into chunks, and store the chunks in the file system +along with some metadata - all while the blockchain is applying new blocks in parallel. + +A more advanced approach might include incremental verification of individual chunks against the +chain app hash, parallel or batched exports, compression, and so on. + +Old snapshots should be removed after some time - generally only the last two snapshots are needed +(to prevent the last one from being removed while a node is restoring it). + +#### Bootstrapping a Node + +An empty node can be state synced by setting the configuration option `statesync.enabled = +true`. The node also needs the chain genesis file for basic chain info, and configuration for +light client verification of the restored snapshot: a set of Tendermint RPC servers, and a +trusted header hash and corresponding height from a trusted source, via the `statesync` +configuration section. + +Once started, the node will connect to the P2P network and begin discovering snapshots. These +will be offered to the local application via the `OfferSnapshot` ABCI method. Once a snapshot +is accepted Tendermint will fetch and apply the snapshot chunks. After all chunks have been +successfully applied, Tendermint verifies the app's `AppHash` against the chain using the light +client, then switches the node to normal consensus operation. + +##### Snapshot Discovery + +When the empty node joins the P2P network, it asks all peers to report snapshots via the +`ListSnapshots` ABCI call (limited to 10 per node). After some time, the node picks the most +suitable snapshot (generally prioritized by height, format, and number of peers), and offers it +to the application via `OfferSnapshot`. The application can choose a number of responses, +including accepting or rejecting it, rejecting the offered format, rejecting the peer who sent +it, and so on. Tendermint will keep discovering and offering snapshots until one is accepted or +the application aborts. + +##### Snapshot Restoration + +Once a snapshot has been accepted via `OfferSnapshot`, Tendermint begins downloading chunks from +any peers that have the same snapshot (i.e. that have identical metadata fields). Chunks are +spooled in a temporary directory, and then given to the application in sequential order via +`ApplySnapshotChunk` until all chunks have been accepted. + +The method for restoring snapshot chunks is entirely up to the application. + +During restoration, the application can respond to `ApplySnapshotChunk` with instructions for how +to continue. This will typically be to accept the chunk and await the next one, but it can also +ask for chunks to be refetched (either the current one or any number of previous ones), P2P peers +to be banned, snapshots to be rejected or retried, and a number of other responses - see the ABCI +reference for details. + +If Tendermint fails to fetch a chunk after some time, it will reject the snapshot and try a +different one via `OfferSnapshot` - the application can choose whether it wants to support +restarting restoration, or simply abort with an error. + +##### Snapshot Verification + +Once all chunks have been accepted, Tendermint issues an `Info` ABCI call to retrieve the +`LastBlockAppHash`. This is compared with the trusted app hash from the chain, retrieved and +verified using the light client. Tendermint also checks that `LastBlockHeight` corresponds to the +height of the snapshot. + +This verification ensures that an application is valid before joining the network. However, the +snapshot restoration may take a long time to complete, so applications may want to employ additional +verification during the restore to detect failures early. This might e.g. include incremental +verification of each chunk against the app hash (using bundled Merkle proofs), checksums to +protect against data corruption by the disk or network, and so on. However, it is important to +note that the only trusted information available is the app hash, and all other snapshot metadata +can be spoofed by adversaries. + +Apps may also want to consider state sync denial-of-service vectors, where adversaries provide +invalid or harmful snapshots to prevent nodes from joining the network. The application can +counteract this by asking Tendermint to ban peers. As a last resort, node operators can use +P2P configuration options to whitelist a set of trusted peers that can provide valid snapshots. + +##### Transition to Consensus + +Once the snapshots have all been restored, Tendermint gathers additional information necessary for +bootstrapping the node (e.g. chain ID, consensus parameters, validator sets, and block headers) +from the genesis file and light client RPC servers. It also calls `Info` to verify the following: + +* that the app hash from the snapshot it has delivered to the Application matches the apphash + stored in the next height's block (in next block execution), or the current block's height + (same block execution) +* that the version that the Application returns in `ResponseInfo` matches the version in the + current height's block header + +Once the state machine has been restored and Tendermint has gathered this additional +information, it transitions to block sync (if enabled) to fetch any remaining blocks up the chain +head, and then transitions to regular consensus operation. At this point the node operates like +any other node, apart from having a truncated block history at the height of the restored snapshot. diff --git a/spec/abci++/abci++_client_server_002_draft.md b/spec/abci++/abci++_client_server_002_draft.md new file mode 100644 index 000000000..f26ee8cd5 --- /dev/null +++ b/spec/abci++/abci++_client_server_002_draft.md @@ -0,0 +1,102 @@ +--- +order: 5 +title: Client and Server +--- + +# Client and Server + +This section is for those looking to implement their own ABCI Server, perhaps in +a new programming language. + +You are expected to have read all previous sections of ABCI++ specification, namely +[Basic Concepts](./abci%2B%2B_basic_concepts_002_draft.md), +[Methods](./abci%2B%2B_methods_002_draft.md), +[Application Requirements](./abci%2B%2B_app_requirements_002_draft.md), and +[Expected Behavior](./abci%2B%2B_tmint_expected_behavior_002_draft.md). + +## Message Protocol and Synchrony + +The message protocol consists of pairs of requests and responses defined in the +[protobuf file](../../proto/tendermint/abci/types.proto). + +Some messages have no fields, while others may include byte-arrays, strings, integers, +or custom protobuf types. + +For more details on protobuf, see the [documentation](https://developers.google.com/protocol-buffers/docs/overview). + +As of v0.36 requests are synchronous. For each of ABCI++'s four connections (see +[Connections](./abci%2B%2B_app_requirements_002_draft.md)), when Tendermint issues a request to the +Application, it will wait for the response before continuing execution. As a side effect, +requests and responses are ordered for each connection, but not necessarily across connections. + +## Server Implementations + +To use ABCI in your programming language of choice, there must be an ABCI +server in that language. Tendermint supports four implementations of the ABCI server: + +- in Tendermint's repository: + - In-process + - ABCI-socket + - GRPC +- [tendermint-rs](https://github.com/informalsystems/tendermint-rs) +- [tower-abci](https://github.com/penumbra-zone/tower-abci) + +The implementations in Tendermint's repository can be tested using `abci-cli` by setting +the `--abci` flag appropriately. + +See examples, in various stages of maintenance, in +[Go](https://github.com/tendermint/tendermint/tree/master/abci/server), +[JavaScript](https://github.com/tendermint/js-abci), +[C++](https://github.com/mdyring/cpp-tmsp), and +[Java](https://github.com/jTendermint/jabci). + +### In Process + +The simplest implementation uses function calls in Golang. +This means ABCI applications written in Golang can be linked with Tendermint Core and run as a single binary. + +### GRPC + +If you are not using Golang, +but [GRPC](https://grpc.io/) is available in your language, this is the easiest approach, +though it will have significant performance overhead. + +Please check GRPC's documentation to know to set up the Application as an +ABCI GRPC server. + +### Socket + +Tendermint's socket-based ABCI interface is an asynchronous, +raw socket server which provides ordered message passing over unix or tcp. +Messages are serialized using Protobuf3 and length-prefixed with a [signed Varint](https://developers.google.com/protocol-buffers/docs/encoding?csw=1#signed-integers). + +If GRPC is not available in your language, your application requires higher +performance, or otherwise enjoy programming, you may implement your own +ABCI server using the Tendermint's socket-based ABCI interface. +The first step is to auto-generate the relevant data +types and codec in your language using `protoc`. +In addition to being proto3 encoded, messages coming over +the socket are length-prefixed. proto3 doesn't have an +official length-prefix standard, so we use our own. The first byte in +the prefix represents the length of the Big Endian encoded length. The +remaining bytes in the prefix are the Big Endian encoded length. + +For example, if the proto3 encoded ABCI message is `0xDEADBEEF` (4 +bytes long), the length-prefixed message is `0x0104DEADBEEF` (`01` byte for encoding the length `04` of the message). If the proto3 +encoded ABCI message is 65535 bytes long, the length-prefixed message +would start with 0x02FFFF. + +Note that this length-prefixing scheme does not apply for GRPC. + +Note that your ABCI server must be able to support multiple connections, as +Tendermint uses four connections. + +## Client + +There are currently two use-cases for an ABCI client. One is testing +tools that allow ABCI requests to be sent to the actual application via +command line. An example of this is `abci-cli`, which accepts CLI commands +to send corresponding ABCI requests. +The other is a consensus engine, such as Tendermint Core, +which makes ABCI requests to the application as prescribed by the consensus +algorithm used. diff --git a/spec/abci++/abci++_methods_002_draft.md b/spec/abci++/abci++_methods_002_draft.md index d1782bbdc..4113a0c58 100644 --- a/spec/abci++/abci++_methods_002_draft.md +++ b/spec/abci++/abci++_methods_002_draft.md @@ -80,13 +80,15 @@ title: Methods * **Usage**: * Called once upon genesis. - * If ResponseInitChain.Validators is empty, the initial validator set will be the RequestInitChain.Validators - * If ResponseInitChain.Validators is not empty, it will be the initial - validator set (regardless of what is in RequestInitChain.Validators). + * If `ResponseInitChain.Validators` is empty, the initial validator set will be the `RequestInitChain.Validators` + * If `ResponseInitChain.Validators` is not empty, it will be the initial + validator set (regardless of what is in `RequestInitChain.Validators`). * This allows the app to decide if it wants to accept the initial validator - set proposed by tendermint (ie. in the genesis file), or if it wants to use - a different one (perhaps computed based on some application specific - information in the genesis file). + set proposed by tendermint (ie. in the genesis file), or if it wants to use + a different one (perhaps computed based on some application specific + information in the genesis file). + * Both `ResponseInitChain.Validators` and `ResponseInitChain.Validators` are [ValidatorUpdate](#validatorupdate) structs. + So, technically, they both are _updating_ the set of validators from the empty set. ### Query @@ -302,7 +304,7 @@ title: Methods |-------------------------|--------------------------------------------------|---------------------------------------------------------------------------------------------|--------------| | tx_records | repeated [TxRecord](#txrecord) | Possibly modified list of transactions that have been picked as part of the proposed block. | 2 | | app_hash | bytes | The Merkle root hash of the application state. | 3 | - | tx_results | repeated [ExecTxResult](#txresult) | List of structures containing the data resulting from executing the transactions | 4 | + | tx_results | repeated [ExecTxResult](#exectxresult) | List of structures containing the data resulting from executing the transactions | 4 | | validator_updates | repeated [ValidatorUpdate](#validatorupdate) | Changes to validator set (set voting power to 0 to remove). | 5 | | consensus_param_updates | [ConsensusParams](#consensusparams) | Changes to consensus-critical gas, size, and other parameters. | 6 | @@ -414,7 +416,7 @@ Note that, if _p_ has a non-`nil` _validValue_, Tendermint will use it as propos |-------------------------|--------------------------------------------------|-----------------------------------------------------------------------------------|--------------| | status | [ProposalStatus](#proposalstatus) | `enum` that signals if the application finds the proposal valid. | 1 | | app_hash | bytes | The Merkle root hash of the application state. | 2 | - | tx_results | repeated [ExecTxResult](#txresult) | List of structures containing the data resulting from executing the transactions. | 3 | + | tx_results | repeated [ExecTxResult](#exectxresult) | List of structures containing the data resulting from executing the transactions. | 3 | | validator_updates | repeated [ValidatorUpdate](#validatorupdate) | Changes to validator set (set voting power to 0 to remove). | 4 | | consensus_param_updates | [ConsensusParams](#consensusparams) | Changes to consensus-critical gas, size, and other parameters. | 5 | @@ -582,7 +584,7 @@ from this condition, but not sure), and _p_ receives a Precommit message for rou | Name | Type | Description | Field Number | |-------------------------|-------------------------------------------------------------|----------------------------------------------------------------------------------|--------------| | events | repeated [Event](abci++_basic_concepts_002_draft.md#events) | Type & Key-Value events for indexing | 1 | - | tx_results | repeated [ExecTxResult](#txresult) | List of structures containing the data resulting from executing the transactions | 2 | + | tx_results | repeated [ExecTxResult](#exectxresult) | List of structures containing the data resulting from executing the transactions | 2 | | validator_updates | repeated [ValidatorUpdate](#validatorupdate) | Changes to validator set (set voting power to 0 to remove). | 3 | | consensus_param_updates | [ConsensusParams](#consensusparams) | Changes to consensus-critical gas, size, and other parameters. | 4 | | app_hash | bytes | The Merkle root hash of the application state. | 5 | diff --git a/test/e2e/node/main.go b/test/e2e/node/main.go index 2cbb9e4b0..94c1af1ab 100644 --- a/test/e2e/node/main.go +++ b/test/e2e/node/main.go @@ -210,7 +210,8 @@ func startLightNode(ctx context.Context, logger log.Logger, cfg *Config) error { // If necessary adjust global WriteTimeout to ensure it's greater than // TimeoutBroadcastTxCommit. // See https://github.com/tendermint/tendermint/issues/3435 - if rpccfg.WriteTimeout <= tmcfg.RPC.TimeoutBroadcastTxCommit { + // Note we don't need to adjust anything if the timeout is already unlimited. + if rpccfg.WriteTimeout > 0 && rpccfg.WriteTimeout <= tmcfg.RPC.TimeoutBroadcastTxCommit { rpccfg.WriteTimeout = tmcfg.RPC.TimeoutBroadcastTxCommit + 1*time.Second } diff --git a/test/e2e/runner/evidence.go b/test/e2e/runner/evidence.go index a71ea14fb..fab1d7b20 100644 --- a/test/e2e/runner/evidence.go +++ b/test/e2e/runner/evidence.go @@ -165,9 +165,9 @@ func generateLightClientAttackEvidence( // create a commit for the forged header blockID := makeBlockID(header.Hash(), 1000, []byte("partshash")) - voteSet := types.NewVoteSet(chainID, forgedHeight, 0, tmproto.SignedMsgType(2), conflictingVals) + voteSet := types.NewExtendedVoteSet(chainID, forgedHeight, 0, tmproto.SignedMsgType(2), conflictingVals) - commit, err := factory.MakeExtendedCommit(ctx, blockID, forgedHeight, 0, voteSet, pv, forgedTime) + ec, err := factory.MakeExtendedCommit(ctx, blockID, forgedHeight, 0, voteSet, pv, forgedTime) if err != nil { return nil, err } @@ -176,7 +176,7 @@ func generateLightClientAttackEvidence( ConflictingBlock: &types.LightBlock{ SignedHeader: &types.SignedHeader{ Header: header, - Commit: commit.StripExtensions(), + Commit: ec.ToCommit(), }, ValidatorSet: conflictingVals, }, diff --git a/test/e2e/tests/app_test.go b/test/e2e/tests/app_test.go index ed041e186..20a153c1d 100644 --- a/test/e2e/tests/app_test.go +++ b/test/e2e/tests/app_test.go @@ -190,6 +190,7 @@ func TestApp_Tx(t *testing.T) { func TestApp_VoteExtensions(t *testing.T) { testNode(t, func(ctx context.Context, t *testing.T, node e2e.Node) { + t.Skip() client, err := node.Client() require.NoError(t, err) diff --git a/types/block.go b/types/block.go index 32a4f9a0a..f508245bb 100644 --- a/types/block.go +++ b/types/block.go @@ -757,22 +757,23 @@ func (ecs ExtendedCommitSig) ValidateBasic() error { if len(ecs.Extension) > MaxVoteExtensionSize { return fmt.Errorf("vote extension is too big (max: %d)", MaxVoteExtensionSize) } - if len(ecs.ExtensionSignature) == 0 { - return errors.New("vote extension signature is missing") - } if len(ecs.ExtensionSignature) > MaxSignatureSize { return fmt.Errorf("vote extension signature is too big (max: %d)", MaxSignatureSize) } return nil } - // We expect there to not be any vote extension or vote extension signature - // on nil or absent votes. - if len(ecs.Extension) != 0 { - return fmt.Errorf("vote extension is present for commit sig with block ID flag %v", ecs.BlockIDFlag) + if len(ecs.ExtensionSignature) == 0 && len(ecs.Extension) != 0 { + return errors.New("vote extension signature absent on vote with extension") } - if len(ecs.ExtensionSignature) != 0 { - return fmt.Errorf("vote extension signature is present for commit sig with block ID flag %v", ecs.BlockIDFlag) + return nil +} + +// EnsureExtensions validates that a vote extensions signature is present for +// this ExtendedCommitSig. +func (ecs ExtendedCommitSig) EnsureExtension() error { + if ecs.BlockIDFlag == BlockIDFlagCommit && len(ecs.ExtensionSignature) == 0 { + return errors.New("vote extension data is missing") } return nil } @@ -916,6 +917,26 @@ func (commit *Commit) Hash() tmbytes.HexBytes { return commit.hash } +// WrappedExtendedCommit wraps a commit as an ExtendedCommit. +// The VoteExtension fields of the resulting value will by nil. +// Wrapping a Commit as an ExtendedCommit is useful when an API +// requires an ExtendedCommit wire type but does not +// need the VoteExtension data. +func (commit *Commit) WrappedExtendedCommit() *ExtendedCommit { + cs := make([]ExtendedCommitSig, len(commit.Signatures)) + for idx, s := range commit.Signatures { + cs[idx] = ExtendedCommitSig{ + CommitSig: s, + } + } + return &ExtendedCommit{ + Height: commit.Height, + Round: commit.Round, + BlockID: commit.BlockID, + ExtendedSignatures: cs, + } +} + // StringIndented returns a string representation of the commit. func (commit *Commit) StringIndented(indent string) string { if commit == nil { @@ -1013,17 +1034,33 @@ func (ec *ExtendedCommit) Clone() *ExtendedCommit { return &ecc } +// ToExtendedVoteSet constructs a VoteSet from the Commit and validator set. +// Panics if signatures from the ExtendedCommit can't be added to the voteset. +// Panics if any of the votes have invalid or absent vote extension data. +// Inverse of VoteSet.MakeExtendedCommit(). +func (ec *ExtendedCommit) ToExtendedVoteSet(chainID string, vals *ValidatorSet) *VoteSet { + voteSet := NewExtendedVoteSet(chainID, ec.Height, ec.Round, tmproto.PrecommitType, vals) + ec.addSigsToVoteSet(voteSet) + return voteSet +} + // ToVoteSet constructs a VoteSet from the Commit and validator set. -// Panics if signatures from the commit can't be added to the voteset. +// Panics if signatures from the ExtendedCommit can't be added to the voteset. // Inverse of VoteSet.MakeExtendedCommit(). func (ec *ExtendedCommit) ToVoteSet(chainID string, vals *ValidatorSet) *VoteSet { voteSet := NewVoteSet(chainID, ec.Height, ec.Round, tmproto.PrecommitType, vals) + ec.addSigsToVoteSet(voteSet) + return voteSet +} + +// addSigsToVoteSet adds all of the signature to voteSet. +func (ec *ExtendedCommit) addSigsToVoteSet(voteSet *VoteSet) { for idx, ecs := range ec.ExtendedSignatures { if ecs.BlockIDFlag == BlockIDFlagAbsent { continue // OK, some precommits can be missing. } vote := ec.GetExtendedVote(int32(idx)) - if err := vote.ValidateWithExtension(); err != nil { + if err := vote.ValidateBasic(); err != nil { panic(fmt.Errorf("failed to validate vote reconstructed from LastCommit: %w", err)) } added, err := voteSet.AddVote(vote) @@ -1031,12 +1068,58 @@ func (ec *ExtendedCommit) ToVoteSet(chainID string, vals *ValidatorSet) *VoteSet panic(fmt.Errorf("failed to reconstruct vote set from extended commit: %w", err)) } } +} + +// ToVoteSet constructs a VoteSet from the Commit and validator set. +// Panics if signatures from the commit can't be added to the voteset. +// Inverse of VoteSet.MakeCommit(). +func (commit *Commit) ToVoteSet(chainID string, vals *ValidatorSet) *VoteSet { + voteSet := NewVoteSet(chainID, commit.Height, commit.Round, tmproto.PrecommitType, vals) + for idx, cs := range commit.Signatures { + if cs.BlockIDFlag == BlockIDFlagAbsent { + continue // OK, some precommits can be missing. + } + vote := commit.GetVote(int32(idx)) + if err := vote.ValidateBasic(); err != nil { + panic(fmt.Errorf("failed to validate vote reconstructed from commit: %w", err)) + } + added, err := voteSet.AddVote(vote) + if !added || err != nil { + panic(fmt.Errorf("failed to reconstruct vote set from commit: %w", err)) + } + } return voteSet } -// StripExtensions converts an ExtendedCommit to a Commit by removing all vote +// EnsureExtensions validates that a vote extensions signature is present for +// every ExtendedCommitSig in the ExtendedCommit. +func (ec *ExtendedCommit) EnsureExtensions() error { + for _, ecs := range ec.ExtendedSignatures { + if err := ecs.EnsureExtension(); err != nil { + return err + } + } + return nil +} + +// StripExtensions removes all VoteExtension data from an ExtendedCommit. This +// is useful when dealing with an ExendedCommit but vote extension data is +// expected to be absent. +func (ec *ExtendedCommit) StripExtensions() bool { + stripped := false + for idx := range ec.ExtendedSignatures { + if len(ec.ExtendedSignatures[idx].Extension) > 0 || len(ec.ExtendedSignatures[idx].ExtensionSignature) > 0 { + stripped = true + } + ec.ExtendedSignatures[idx].Extension = nil + ec.ExtendedSignatures[idx].ExtensionSignature = nil + } + return stripped +} + +// ToCommit converts an ExtendedCommit to a Commit by removing all vote // extension-related fields. -func (ec *ExtendedCommit) StripExtensions() *Commit { +func (ec *ExtendedCommit) ToCommit() *Commit { cs := make([]CommitSig, len(ec.ExtendedSignatures)) for idx, ecs := range ec.ExtendedSignatures { cs[idx] = ecs.CommitSig diff --git a/types/block_test.go b/types/block_test.go index 09a8b602e..4c3a74d8f 100644 --- a/types/block_test.go +++ b/types/block_test.go @@ -49,7 +49,7 @@ func TestBlockAddEvidence(t *testing.T) { require.NoError(t, err) evList := []Evidence{ev} - block := MakeBlock(h, txs, extCommit.StripExtensions(), evList) + block := MakeBlock(h, txs, extCommit.ToCommit(), evList) require.NotNil(t, block) require.Equal(t, 1, len(block.Evidence)) require.NotNil(t, block.EvidenceHash) @@ -68,7 +68,7 @@ func TestBlockValidateBasic(t *testing.T) { voteSet, valSet, vals := randVoteSet(ctx, t, h-1, 1, tmproto.PrecommitType, 10, 1) extCommit, err := makeExtCommit(ctx, lastID, h-1, 1, voteSet, vals, time.Now()) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() ev, err := NewMockDuplicateVoteEvidenceWithValidator(ctx, h, time.Now(), vals[0], "block-test-chain") require.NoError(t, err) @@ -163,7 +163,7 @@ func TestBlockMakePartSetWithEvidence(t *testing.T) { require.NoError(t, err) evList := []Evidence{ev} - partSet, err := MakeBlock(h, []Tx{Tx("Hello World")}, extCommit.StripExtensions(), evList).MakePartSet(512) + partSet, err := MakeBlock(h, []Tx{Tx("Hello World")}, extCommit.ToCommit(), evList).MakePartSet(512) require.NoError(t, err) assert.NotNil(t, partSet) @@ -187,7 +187,7 @@ func TestBlockHashesTo(t *testing.T) { require.NoError(t, err) evList := []Evidence{ev} - block := MakeBlock(h, []Tx{Tx("Hello World")}, extCommit.StripExtensions(), evList) + block := MakeBlock(h, []Tx{Tx("Hello World")}, extCommit.ToCommit(), evList) block.ValidatorsHash = valSet.Hash() assert.False(t, block.HashesTo([]byte{})) assert.False(t, block.HashesTo([]byte("something else"))) @@ -483,7 +483,7 @@ func randCommit(ctx context.Context, t *testing.T, now time.Time) *Commit { require.NoError(t, err) - return commit.StripExtensions() + return commit.ToCommit() } func hexBytesFromString(t *testing.T, s string) bytes.HexBytes { @@ -556,33 +556,138 @@ func TestBlockMaxDataBytesNoEvidence(t *testing.T) { } } +// TestVoteSetToExtendedCommit tests that the extended commit produced from a +// vote set contains the same vote information as the vote set. The test ensures +// that the MakeExtendedCommit method behaves as expected, whether vote extensions +// are present in the original votes or not. +func TestVoteSetToExtendedCommit(t *testing.T) { + for _, testCase := range []struct { + name string + includeExtension bool + }{ + { + name: "no extensions", + includeExtension: false, + }, + { + name: "with extensions", + includeExtension: true, + }, + } { + + t.Run(testCase.name, func(t *testing.T) { + blockID := makeBlockIDRandom() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + valSet, vals := randValidatorPrivValSet(ctx, t, 10, 1) + var voteSet *VoteSet + if testCase.includeExtension { + voteSet = NewExtendedVoteSet("test_chain_id", 3, 1, tmproto.PrecommitType, valSet) + } else { + voteSet = NewVoteSet("test_chain_id", 3, 1, tmproto.PrecommitType, valSet) + } + for i := 0; i < len(vals); i++ { + pubKey, err := vals[i].GetPubKey(ctx) + require.NoError(t, err) + vote := &Vote{ + ValidatorAddress: pubKey.Address(), + ValidatorIndex: int32(i), + Height: 3, + Round: 1, + Type: tmproto.PrecommitType, + BlockID: blockID, + Timestamp: time.Now(), + } + v := vote.ToProto() + err = vals[i].SignVote(ctx, voteSet.ChainID(), v) + require.NoError(t, err) + vote.Signature = v.Signature + if testCase.includeExtension { + vote.ExtensionSignature = v.ExtensionSignature + } + added, err := voteSet.AddVote(vote) + require.NoError(t, err) + require.True(t, added) + } + ec := voteSet.MakeExtendedCommit() + + for i := int32(0); int(i) < len(vals); i++ { + vote1 := voteSet.GetByIndex(i) + vote2 := ec.GetExtendedVote(i) + + vote1bz, err := vote1.ToProto().Marshal() + require.NoError(t, err) + vote2bz, err := vote2.ToProto().Marshal() + require.NoError(t, err) + assert.Equal(t, vote1bz, vote2bz) + } + }) + } +} + +// TestExtendedCommitToVoteSet tests that the vote set produced from an extended commit +// contains the same vote information as the extended commit. The test ensures +// that the ToVoteSet method behaves as expected, whether vote extensions +// are present in the original votes or not. func TestExtendedCommitToVoteSet(t *testing.T) { - lastID := makeBlockIDRandom() - h := int64(3) + for _, testCase := range []struct { + name string + includeExtension bool + }{ + { + name: "no extensions", + includeExtension: false, + }, + { + name: "with extensions", + includeExtension: true, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + lastID := makeBlockIDRandom() + h := int64(3) - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() - voteSet, valSet, vals := randVoteSet(ctx, t, h-1, 1, tmproto.PrecommitType, 10, 1) - extCommit, err := makeExtCommit(ctx, lastID, h-1, 1, voteSet, vals, time.Now()) - assert.NoError(t, err) + voteSet, valSet, vals := randVoteSet(ctx, t, h-1, 1, tmproto.PrecommitType, 10, 1) + extCommit, err := makeExtCommit(ctx, lastID, h-1, 1, voteSet, vals, time.Now()) + assert.NoError(t, err) - chainID := voteSet.ChainID() - voteSet2 := extCommit.ToVoteSet(chainID, valSet) + if !testCase.includeExtension { + for i := 0; i < len(vals); i++ { + v := voteSet.GetByIndex(int32(i)) + v.Extension = nil + v.ExtensionSignature = nil + extCommit.ExtendedSignatures[i].Extension = nil + extCommit.ExtendedSignatures[i].ExtensionSignature = nil + } + } - for i := int32(0); int(i) < len(vals); i++ { - vote1 := voteSet.GetByIndex(i) - vote2 := voteSet2.GetByIndex(i) - vote3 := extCommit.GetExtendedVote(i) + chainID := voteSet.ChainID() + var voteSet2 *VoteSet + if testCase.includeExtension { + voteSet2 = extCommit.ToExtendedVoteSet(chainID, valSet) + } else { + voteSet2 = extCommit.ToVoteSet(chainID, valSet) + } - vote1bz, err := vote1.ToProto().Marshal() - require.NoError(t, err) - vote2bz, err := vote2.ToProto().Marshal() - require.NoError(t, err) - vote3bz, err := vote3.ToProto().Marshal() - require.NoError(t, err) - assert.Equal(t, vote1bz, vote2bz) - assert.Equal(t, vote1bz, vote3bz) + for i := int32(0); int(i) < len(vals); i++ { + vote1 := voteSet.GetByIndex(i) + vote2 := voteSet2.GetByIndex(i) + vote3 := extCommit.GetExtendedVote(i) + + vote1bz, err := vote1.ToProto().Marshal() + require.NoError(t, err) + vote2bz, err := vote2.ToProto().Marshal() + require.NoError(t, err) + vote3bz, err := vote3.ToProto().Marshal() + require.NoError(t, err) + assert.Equal(t, vote1bz, vote2bz) + assert.Equal(t, vote1bz, vote3bz) + } + }) } } @@ -637,7 +742,7 @@ func TestCommitToVoteSetWithVotesForNilBlock(t *testing.T) { if tc.valid { extCommit := voteSet.MakeExtendedCommit() // panics without > 2/3 valid votes assert.NotNil(t, extCommit) - err := valSet.VerifyCommit(voteSet.ChainID(), blockID, height-1, extCommit.StripExtensions()) + err := valSet.VerifyCommit(voteSet.ChainID(), blockID, height-1, extCommit.ToCommit()) assert.NoError(t, err) } else { assert.Panics(t, func() { voteSet.MakeExtendedCommit() }) diff --git a/types/evidence_test.go b/types/evidence_test.go index 8b06a6218..d014a3ecc 100644 --- a/types/evidence_test.go +++ b/types/evidence_test.go @@ -155,7 +155,7 @@ func TestLightClientAttackEvidenceBasic(t *testing.T) { blockID := makeBlockID(crypto.Checksum([]byte("blockhash")), math.MaxInt32, crypto.Checksum([]byte("partshash"))) extCommit, err := makeExtCommit(ctx, blockID, height, 1, voteSet, privVals, defaultVoteTime) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() lcae := &LightClientAttackEvidence{ ConflictingBlock: &LightBlock{ @@ -221,7 +221,7 @@ func TestLightClientAttackEvidenceValidation(t *testing.T) { blockID := makeBlockID(header.Hash(), math.MaxInt32, crypto.Checksum([]byte("partshash"))) extCommit, err := makeExtCommit(ctx, blockID, height, 1, voteSet, privVals, time.Now()) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() lcae := &LightClientAttackEvidence{ ConflictingBlock: &LightBlock{ @@ -434,7 +434,7 @@ func TestEvidenceVectors(t *testing.T) { ConflictingBlock: &LightBlock{ SignedHeader: &SignedHeader{ Header: header, - Commit: extCommit.StripExtensions(), + Commit: extCommit.ToCommit(), }, ValidatorSet: valSet, }, diff --git a/types/params.go b/types/params.go index e8ee6fcdf..3b5e9a250 100644 --- a/types/params.go +++ b/types/params.go @@ -43,6 +43,7 @@ type ConsensusParams struct { Version VersionParams `json:"version"` Synchrony SynchronyParams `json:"synchrony"` Timeout TimeoutParams `json:"timeout"` + ABCI ABCIParams `json:"abci"` } // HashedParams is a subset of ConsensusParams. @@ -96,6 +97,21 @@ type TimeoutParams struct { BypassCommitTimeout bool `json:"bypass_commit_timeout"` } +// ABCIParams configure ABCI functionality specific to the Application Blockchain +// Interface. +type ABCIParams struct { + VoteExtensionsEnableHeight int64 `json:"vote_extensions_enable_height"` +} + +// VoteExtensionsEnabled returns true if vote extensions are enabled at height h +// and false otherwise. +func (a ABCIParams) VoteExtensionsEnabled(h int64) bool { + if a.VoteExtensionsEnableHeight == 0 { + return false + } + return a.VoteExtensionsEnableHeight <= h +} + // DefaultConsensusParams returns a default ConsensusParams. func DefaultConsensusParams() *ConsensusParams { return &ConsensusParams{ @@ -105,6 +121,7 @@ func DefaultConsensusParams() *ConsensusParams { Version: DefaultVersionParams(), Synchrony: DefaultSynchronyParams(), Timeout: DefaultTimeoutParams(), + ABCI: DefaultABCIParams(), } } @@ -176,6 +193,13 @@ func DefaultTimeoutParams() TimeoutParams { } } +func DefaultABCIParams() ABCIParams { + return ABCIParams{ + // When set to 0, vote extensions are not required. + VoteExtensionsEnableHeight: 0, + } +} + // TimeoutParamsOrDefaults returns the SynchronyParams, filling in any zero values // with the Tendermint defined default values. func (t TimeoutParams) TimeoutParamsOrDefaults() TimeoutParams { diff --git a/types/validation_test.go b/types/validation_test.go index f63c34450..c28f63000 100644 --- a/types/validation_test.go +++ b/types/validation_test.go @@ -153,7 +153,7 @@ func TestValidatorSet_VerifyCommit_CheckAllSignatures(t *testing.T) { voteSet, valSet, vals := randVoteSet(ctx, t, h, 0, tmproto.PrecommitType, 4, 10) extCommit, err := makeExtCommit(ctx, blockID, h, 0, voteSet, vals, time.Now()) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() require.NoError(t, valSet.VerifyCommit(chainID, blockID, h, commit)) @@ -184,7 +184,7 @@ func TestValidatorSet_VerifyCommitLight_ReturnsAsSoonAsMajorityOfVotingPowerSign voteSet, valSet, vals := randVoteSet(ctx, t, h, 0, tmproto.PrecommitType, 4, 10) extCommit, err := makeExtCommit(ctx, blockID, h, 0, voteSet, vals, time.Now()) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() require.NoError(t, valSet.VerifyCommit(chainID, blockID, h, commit)) @@ -212,7 +212,7 @@ func TestValidatorSet_VerifyCommitLightTrusting_ReturnsAsSoonAsTrustLevelOfVotin voteSet, valSet, vals := randVoteSet(ctx, t, h, 0, tmproto.PrecommitType, 4, 10) extCommit, err := makeExtCommit(ctx, blockID, h, 0, voteSet, vals, time.Now()) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() require.NoError(t, valSet.VerifyCommit(chainID, blockID, h, commit)) @@ -239,7 +239,7 @@ func TestValidatorSet_VerifyCommitLightTrusting(t *testing.T) { newValSet, _ = randValidatorPrivValSet(ctx, t, 2, 1) ) require.NoError(t, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() testCases := []struct { valSet *ValidatorSet @@ -284,7 +284,7 @@ func TestValidatorSet_VerifyCommitLightTrustingErrorsOnOverflow(t *testing.T) { ) require.NoError(t, err) - err = valSet.VerifyCommitLightTrusting("test_chain_id", extCommit.StripExtensions(), + err = valSet.VerifyCommitLightTrusting("test_chain_id", extCommit.ToCommit(), tmmath.Fraction{Numerator: 25, Denominator: 55}) if assert.Error(t, err) { assert.Contains(t, err.Error(), "int64 overflow") diff --git a/types/validator_set_test.go b/types/validator_set_test.go index 8b5846da9..81e81919d 100644 --- a/types/validator_set_test.go +++ b/types/validator_set_test.go @@ -1541,7 +1541,7 @@ func BenchmarkValidatorSet_VerifyCommit_Ed25519(b *testing.B) { // nolint // create a commit with n validators extCommit, err := makeExtCommit(ctx, blockID, h, 0, voteSet, vals, time.Now()) require.NoError(b, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() for i := 0; i < b.N/n; i++ { err = valSet.VerifyCommit(chainID, blockID, h, commit) @@ -1570,7 +1570,7 @@ func BenchmarkValidatorSet_VerifyCommitLight_Ed25519(b *testing.B) { // nolint // create a commit with n validators extCommit, err := makeExtCommit(ctx, blockID, h, 0, voteSet, vals, time.Now()) require.NoError(b, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() for i := 0; i < b.N/n; i++ { err = valSet.VerifyCommitLight(chainID, blockID, h, commit) @@ -1598,7 +1598,7 @@ func BenchmarkValidatorSet_VerifyCommitLightTrusting_Ed25519(b *testing.B) { // create a commit with n validators extCommit, err := makeExtCommit(ctx, blockID, h, 0, voteSet, vals, time.Now()) require.NoError(b, err) - commit := extCommit.StripExtensions() + commit := extCommit.ToCommit() for i := 0; i < b.N/n; i++ { err = valSet.VerifyCommitLightTrusting(chainID, commit, tmmath.Fraction{Numerator: 1, Denominator: 3}) diff --git a/types/vote.go b/types/vote.go index 446de130a..f7006b8cd 100644 --- a/types/vote.go +++ b/types/vote.go @@ -27,7 +27,7 @@ var ( ErrVoteInvalidBlockHash = errors.New("invalid block hash") ErrVoteNonDeterministicSignature = errors.New("non-deterministic signature") ErrVoteNil = errors.New("nil vote") - ErrVoteInvalidExtension = errors.New("invalid vote extension") + ErrVoteExtensionAbsent = errors.New("vote extension absent") ) type ErrVoteConflictingVotes struct { @@ -112,6 +112,16 @@ func (vote *Vote) CommitSig() CommitSig { } } +// StripExtension removes any extension data from the vote. Useful if the +// chain has not enabled vote extensions. +// Returns true if extension data was present before stripping and false otherwise. +func (vote *Vote) StripExtension() bool { + stripped := len(vote.Extension) > 0 || len(vote.ExtensionSignature) > 0 + vote.Extension = nil + vote.ExtensionSignature = nil + return stripped +} + // ExtendedCommitSig attempts to construct an ExtendedCommitSig from this vote. // Panics if either the vote extension signature is missing or if the block ID // is not either empty or complete. @@ -120,13 +130,8 @@ func (vote *Vote) ExtendedCommitSig() ExtendedCommitSig { return NewExtendedCommitSigAbsent() } - cs := vote.CommitSig() - if vote.BlockID.IsComplete() && len(vote.ExtensionSignature) == 0 { - panic(fmt.Sprintf("Invalid vote %v - BlockID is complete but missing vote extension signature", vote)) - } - return ExtendedCommitSig{ - CommitSig: cs, + CommitSig: vote.CommitSig(), Extension: vote.Extension, ExtensionSignature: vote.ExtensionSignature, } @@ -230,11 +235,11 @@ func (vote *Vote) Verify(chainID string, pubKey crypto.PubKey) error { return err } -// VerifyWithExtension performs the same verification as Verify, but +// VerifyVoteAndExtension performs the same verification as Verify, but // additionally checks whether the vote extension signature corresponds to the // given chain ID and public key. We only verify vote extension signatures for // precommits. -func (vote *Vote) VerifyWithExtension(chainID string, pubKey crypto.PubKey) error { +func (vote *Vote) VerifyVoteAndExtension(chainID string, pubKey crypto.PubKey) error { v, err := vote.verifyAndReturnProto(chainID, pubKey) if err != nil { return err @@ -249,6 +254,20 @@ func (vote *Vote) VerifyWithExtension(chainID string, pubKey crypto.PubKey) erro return nil } +// VerifyExtension checks whether the vote extension signature corresponds to the +// given chain ID and public key. +func (vote *Vote) VerifyExtension(chainID string, pubKey crypto.PubKey) error { + if vote.Type != tmproto.PrecommitType || vote.BlockID.IsNil() { + return nil + } + v := vote.ToProto() + extSignBytes := VoteExtensionSignBytes(chainID, v) + if !pubKey.VerifySignature(extSignBytes, vote.ExtensionSignature) { + return ErrVoteInvalidSignature + } + return nil +} + // ValidateBasic checks whether the vote is well-formed. It does not, however, // check vote extensions - for vote validation with vote extension validation, // use ValidateWithExtension. @@ -306,30 +325,34 @@ func (vote *Vote) ValidateBasic() error { } } - return nil -} - -// ValidateWithExtension performs the same validations as ValidateBasic, but -// additionally checks whether a vote extension signature is present. This -// function is used in places where vote extension signatures are expected. -func (vote *Vote) ValidateWithExtension() error { - if err := vote.ValidateBasic(); err != nil { - return err - } - - // We should always see vote extension signatures in non-nil precommits if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsNil() { - if len(vote.ExtensionSignature) == 0 { - return errors.New("vote extension signature is missing") - } if len(vote.ExtensionSignature) > MaxSignatureSize { return fmt.Errorf("vote extension signature is too big (max: %d)", MaxSignatureSize) } + if len(vote.ExtensionSignature) == 0 && len(vote.Extension) != 0 { + return fmt.Errorf("vote extension signature absent on vote with extension") + } } return nil } +// EnsureExtension checks for the presence of extensions signature data +// on precommit vote types. +func (vote *Vote) EnsureExtension() error { + // We should always see vote extension signatures in non-nil precommits + if vote.Type != tmproto.PrecommitType { + return nil + } + if vote.BlockID.IsNil() { + return nil + } + if len(vote.ExtensionSignature) > 0 { + return nil + } + return ErrVoteExtensionAbsent +} + // ToProto converts the handwritten type to proto generated type // return type, nil if everything converts safely, otherwise nil, error func (vote *Vote) ToProto() *tmproto.Vote { diff --git a/types/vote_set.go b/types/vote_set.go index 224d4e4f8..6d83ac85d 100644 --- a/types/vote_set.go +++ b/types/vote_set.go @@ -3,6 +3,7 @@ package types import ( "bytes" "encoding/json" + "errors" "fmt" "strings" "sync" @@ -53,11 +54,12 @@ const ( NOTE: Assumes that the sum total of voting power does not exceed MaxUInt64. */ type VoteSet struct { - chainID string - height int64 - round int32 - signedMsgType tmproto.SignedMsgType - valSet *ValidatorSet + chainID string + height int64 + round int32 + signedMsgType tmproto.SignedMsgType + valSet *ValidatorSet + extensionsEnabled bool mtx sync.Mutex votesBitArray *bits.BitArray @@ -68,7 +70,8 @@ type VoteSet struct { peerMaj23s map[string]BlockID // Maj23 for each peer } -// Constructs a new VoteSet struct used to accumulate votes for given height/round. +// NewVoteSet instantiates all fields of a new vote set. This constructor requires +// that no vote extension data be present on the votes that are added to the set. func NewVoteSet(chainID string, height int64, round int32, signedMsgType tmproto.SignedMsgType, valSet *ValidatorSet) *VoteSet { if height == 0 { @@ -89,6 +92,16 @@ func NewVoteSet(chainID string, height int64, round int32, } } +// NewExtendedVoteSet constructs a vote set with additional vote verification logic. +// The VoteSet constructed with NewExtendedVoteSet verifies the vote extension +// data for every vote added to the set. +func NewExtendedVoteSet(chainID string, height int64, round int32, + signedMsgType tmproto.SignedMsgType, valSet *ValidatorSet) *VoteSet { + vs := NewVoteSet(chainID, height, round, signedMsgType, valSet) + vs.extensionsEnabled = true + return vs +} + func (voteSet *VoteSet) ChainID() string { return voteSet.chainID } @@ -194,8 +207,17 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) { } // Check signature. - if err := vote.VerifyWithExtension(voteSet.chainID, val.PubKey); err != nil { - return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) + if voteSet.extensionsEnabled { + if err := vote.VerifyVoteAndExtension(voteSet.chainID, val.PubKey); err != nil { + return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) + } + } else { + if err := vote.Verify(voteSet.chainID, val.PubKey); err != nil { + return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err) + } + if len(vote.ExtensionSignature) > 0 || len(vote.Extension) > 0 { + return false, errors.New("unexpected vote extension data present in vote") + } } // Add vote and get conflicting vote if any. diff --git a/types/vote_set_test.go b/types/vote_set_test.go index 8d166d508..e35da7491 100644 --- a/types/vote_set_test.go +++ b/types/vote_set_test.go @@ -498,6 +498,92 @@ func TestVoteSet_MakeCommit(t *testing.T) { } } +// TestVoteSet_VoteExtensionsEnabled tests that the vote set correctly validates +// vote extensions data when either required or not required. +func TestVoteSet_VoteExtensionsEnabled(t *testing.T) { + for _, tc := range []struct { + name string + requireExtensions bool + addExtension bool + exepectError bool + }{ + { + name: "no extension but expected", + requireExtensions: true, + addExtension: false, + exepectError: true, + }, + { + name: "invalid extensions but not expected", + requireExtensions: true, + addExtension: false, + exepectError: true, + }, + { + name: "no extension and not expected", + requireExtensions: false, + addExtension: false, + exepectError: false, + }, + { + name: "extension and expected", + requireExtensions: true, + addExtension: true, + exepectError: false, + }, + } { + t.Run(tc.name, func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + height, round := int64(1), int32(0) + valSet, privValidators := randValidatorPrivValSet(ctx, t, 5, 10) + var voteSet *VoteSet + if tc.requireExtensions { + voteSet = NewExtendedVoteSet("test_chain_id", height, round, tmproto.PrecommitType, valSet) + } else { + voteSet = NewVoteSet("test_chain_id", height, round, tmproto.PrecommitType, valSet) + } + + val0 := privValidators[0] + + val0p, err := val0.GetPubKey(ctx) + require.NoError(t, err) + val0Addr := val0p.Address() + blockHash := crypto.CRandBytes(32) + blockPartsTotal := uint32(123) + blockPartSetHeader := PartSetHeader{blockPartsTotal, crypto.CRandBytes(32)} + + vote := &Vote{ + ValidatorAddress: val0Addr, + ValidatorIndex: 0, + Height: height, + Round: round, + Type: tmproto.PrecommitType, + Timestamp: tmtime.Now(), + BlockID: BlockID{blockHash, blockPartSetHeader}, + } + v := vote.ToProto() + err = val0.SignVote(ctx, voteSet.ChainID(), v) + require.NoError(t, err) + vote.Signature = v.Signature + + if tc.addExtension { + vote.ExtensionSignature = v.ExtensionSignature + } + + added, err := voteSet.AddVote(vote) + if tc.exepectError { + require.Error(t, err) + require.False(t, added) + } else { + require.NoError(t, err) + require.True(t, added) + } + }) + } +} + // NOTE: privValidators are in order func randVoteSet( ctx context.Context, @@ -510,7 +596,7 @@ func randVoteSet( ) (*VoteSet, *ValidatorSet, []PrivValidator) { t.Helper() valSet, privValidators := randValidatorPrivValSet(ctx, t, numValidators, votingPower) - return NewVoteSet("test_chain_id", height, round, signedMsgType, valSet), valSet, privValidators + return NewExtendedVoteSet("test_chain_id", height, round, signedMsgType, valSet), valSet, privValidators } func deterministicVoteSet( @@ -523,7 +609,7 @@ func deterministicVoteSet( ) (*VoteSet, *ValidatorSet, []PrivValidator) { t.Helper() valSet, privValidators := deterministicValidatorSet(ctx, t) - return NewVoteSet("test_chain_id", height, round, signedMsgType, valSet), valSet, privValidators + return NewExtendedVoteSet("test_chain_id", height, round, signedMsgType, valSet), valSet, privValidators } func randValidatorPrivValSet(ctx context.Context, t testing.TB, numValidators int, votingPower int64) (*ValidatorSet, []PrivValidator) { diff --git a/types/vote_test.go b/types/vote_test.go index 70cd91381..d0819d7c4 100644 --- a/types/vote_test.go +++ b/types/vote_test.go @@ -267,7 +267,7 @@ func TestVoteExtension(t *testing.T) { if tc.includeSignature { vote.ExtensionSignature = v.ExtensionSignature } - err = vote.VerifyWithExtension("test_chain_id", pk) + err = vote.VerifyExtension("test_chain_id", pk) if tc.expectError { require.Error(t, err) } else { @@ -361,7 +361,7 @@ func TestValidVotes(t *testing.T) { signVote(ctx, t, privVal, "test_chain_id", tc.vote) tc.malleateVote(tc.vote) require.NoError(t, tc.vote.ValidateBasic(), "ValidateBasic for %s", tc.name) - require.NoError(t, tc.vote.ValidateWithExtension(), "ValidateWithExtension for %s", tc.name) + require.NoError(t, tc.vote.EnsureExtension(), "EnsureExtension for %s", tc.name) } } @@ -387,13 +387,13 @@ func TestInvalidVotes(t *testing.T) { signVote(ctx, t, privVal, "test_chain_id", prevote) tc.malleateVote(prevote) require.Error(t, prevote.ValidateBasic(), "ValidateBasic for %s in invalid prevote", tc.name) - require.Error(t, prevote.ValidateWithExtension(), "ValidateWithExtension for %s in invalid prevote", tc.name) + require.NoError(t, prevote.EnsureExtension(), "EnsureExtension for %s in invalid prevote", tc.name) precommit := examplePrecommit(t) signVote(ctx, t, privVal, "test_chain_id", precommit) tc.malleateVote(precommit) require.Error(t, precommit.ValidateBasic(), "ValidateBasic for %s in invalid precommit", tc.name) - require.Error(t, precommit.ValidateWithExtension(), "ValidateWithExtension for %s in invalid precommit", tc.name) + require.NoError(t, precommit.EnsureExtension(), "EnsureExtension for %s in invalid precommit", tc.name) } } @@ -414,7 +414,7 @@ func TestInvalidPrevotes(t *testing.T) { signVote(ctx, t, privVal, "test_chain_id", prevote) tc.malleateVote(prevote) require.Error(t, prevote.ValidateBasic(), "ValidateBasic for %s", tc.name) - require.Error(t, prevote.ValidateWithExtension(), "ValidateWithExtension for %s", tc.name) + require.NoError(t, prevote.EnsureExtension(), "EnsureExtension for %s", tc.name) } } @@ -431,18 +431,44 @@ func TestInvalidPrecommitExtensions(t *testing.T) { v.Extension = []byte("extension") v.ExtensionSignature = nil }}, - // TODO(thane): Re-enable once https://github.com/tendermint/tendermint/issues/8272 is resolved - //{"missing vote extension signature", func(v *Vote) { v.ExtensionSignature = nil }}, {"oversized vote extension signature", func(v *Vote) { v.ExtensionSignature = make([]byte, MaxSignatureSize+1) }}, } for _, tc := range testCases { precommit := examplePrecommit(t) signVote(ctx, t, privVal, "test_chain_id", precommit) tc.malleateVote(precommit) - // We don't expect an error from ValidateBasic, because it doesn't - // handle vote extensions. - require.NoError(t, precommit.ValidateBasic(), "ValidateBasic for %s", tc.name) - require.Error(t, precommit.ValidateWithExtension(), "ValidateWithExtension for %s", tc.name) + // ValidateBasic ensures that vote extensions, if present, are well formed + require.Error(t, precommit.ValidateBasic(), "ValidateBasic for %s", tc.name) + } +} + +func TestEnsureVoteExtension(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + privVal := NewMockPV() + + testCases := []struct { + name string + malleateVote func(*Vote) + expectError bool + }{ + {"vote extension signature absent", func(v *Vote) { + v.Extension = nil + v.ExtensionSignature = nil + }, true}, + {"vote extension signature present", func(v *Vote) { + v.ExtensionSignature = []byte("extension signature") + }, false}, + } + for _, tc := range testCases { + precommit := examplePrecommit(t) + signVote(ctx, t, privVal, "test_chain_id", precommit) + tc.malleateVote(precommit) + if tc.expectError { + require.Error(t, precommit.EnsureExtension(), "EnsureExtension for %s", tc.name) + } else { + require.NoError(t, precommit.EnsureExtension(), "EnsureExtension for %s", tc.name) + } } } @@ -497,7 +523,7 @@ func getSampleCommit(ctx context.Context, t testing.TB) *Commit { require.NoError(t, err) - return commit.StripExtensions() + return commit.ToCommit() } func BenchmarkVoteSignBytes(b *testing.B) {