From b89caeb7e91737a08a486ede0cd7f35040b98f6e Mon Sep 17 00:00:00 2001 From: Paolo Galli Date: Thu, 25 Jul 2024 17:47:27 +0200 Subject: [PATCH 01/12] feature: add justified revision for block --- api/blocks/blocks_test.go | 13 ++++++++++++ api/utils/revisions.go | 7 +++++++ api/utils/revisions_test.go | 10 +++++++++ bft/engine.go | 39 +++++++++++++++++++++++++++++++++++- bft/engine_test.go | 16 +++++++++++++++ cmd/thor/node/node.go | 6 ++++++ cmd/thor/node/packer_loop.go | 6 ++++++ cmd/thor/solo/types.go | 6 ++++++ 8 files changed, 102 insertions(+), 1 deletion(-) diff --git a/api/blocks/blocks_test.go b/api/blocks/blocks_test.go index 2bad4f203..d7cd786ca 100644 --- a/api/blocks/blocks_test.go +++ b/api/blocks/blocks_test.go @@ -52,6 +52,7 @@ func TestBlock(t *testing.T) { "testGetBlockByHeight": testGetBlockByHeight, "testGetBestBlock": testGetBestBlock, "testGetFinalizedBlock": testGetFinalizedBlock, + "testGetJustifiedBlock": testGetJustifiedBlock, "testGetBlockWithRevisionNumberTooHigh": testGetBlockWithRevisionNumberTooHigh, } { t.Run(name, tt) @@ -99,6 +100,18 @@ func testGetFinalizedBlock(t *testing.T) { assert.Equal(t, genesisBlock.Header().ID(), finalized.ID) } +func testGetJustifiedBlock(t *testing.T) { + res, statusCode := httpGet(t, ts.URL+"/blocks/justified") + justified := new(blocks.JSONCollapsedBlock) + if err := json.Unmarshal(res, &justified); err != nil { + t.Fatal(err) + } + + assert.Equal(t, http.StatusOK, statusCode) + assert.Equal(t, uint32(0), justified.Number) + assert.Equal(t, genesisBlock.Header().ID(), justified.ID) +} + func testGetBlockById(t *testing.T) { res, statusCode := httpGet(t, ts.URL+"/blocks/"+blk.Header().ID().String()) rb := new(blocks.JSONCollapsedBlock) diff --git a/api/utils/revisions.go b/api/utils/revisions.go index c4cf668cb..0b2b380e7 100644 --- a/api/utils/revisions.go +++ b/api/utils/revisions.go @@ -21,6 +21,7 @@ const ( revBest int64 = -1 revFinalized int64 = -2 revNext int64 = -3 + revJustified int64 = -4 ) type Revision struct { @@ -41,6 +42,10 @@ func ParseRevision(revision string, allowNext bool) (*Revision, error) { return &Revision{revFinalized}, nil } + if revision == "justified" { + return &Revision{revJustified}, nil + } + if revision == "next" { if !allowNext { return nil, errors.New("invalid revision: next is not allowed") @@ -83,6 +88,8 @@ func GetSummary(rev *Revision, repo *chain.Repository, bft bft.Finalizer) (sum * id = repo.BestBlockSummary().Header.ID() case revFinalized: id = bft.Finalized() + case revJustified: + id = bft.Justified() } } if id.IsZero() { diff --git a/api/utils/revisions_test.go b/api/utils/revisions_test.go index 3af996693..76a4bbfdc 100644 --- a/api/utils/revisions_test.go +++ b/api/utils/revisions_test.go @@ -41,6 +41,11 @@ func TestParseRevision(t *testing.T) { err: nil, expected: &Revision{revBest}, }, + { + revision: "justified", + err: nil, + expected: &Revision{revJustified}, + }, { revision: "finalized", err: nil, @@ -122,6 +127,11 @@ func TestGetSummary(t *testing.T) { revision: &Revision{uint32(1234)}, err: errors.New("not found"), }, + { + name: "justified", + revision: &Revision{revJustified}, + err: nil, + }, { name: "finalized", revision: &Revision{revFinalized}, diff --git a/bft/engine.go b/bft/engine.go index 2e3f6a73f..cff59e01d 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -24,9 +24,11 @@ import ( const dataStoreName = "bft.engine" var finalizedKey = []byte("finalized") +var justifiedKey = []byte("justified") type Finalizer interface { Finalized() thor.Bytes32 + Justified() thor.Bytes32 } // BFTEngine tracks all votes of blocks, computes the finalized checkpoint. @@ -39,6 +41,7 @@ type BFTEngine struct { master thor.Address casts casts finalized atomic.Value + justified atomic.Value caches struct { state *lru.Cache quality *lru.Cache @@ -60,6 +63,17 @@ func NewEngine(repo *chain.Repository, mainDB *muxdb.MuxDB, forkConfig thor.Fork engine.caches.quality, _ = lru.New(16) engine.caches.justifier = cache.NewPrioCache(16) + // Restore justified block, if any + if val, err := engine.data.Get(justifiedKey); err != nil { + if !engine.data.IsNotFound(err) { + return nil, err + } + engine.justified.Store(engine.repo.GenesisBlock().Header().ID()) + } else { + engine.justified.Store(thor.BytesToBytes32(val)) + } + + // Restore finalized block, if any if val, err := engine.data.Get(finalizedKey); err != nil { if !engine.data.IsNotFound(err) { return nil, err @@ -77,6 +91,10 @@ func (engine *BFTEngine) Finalized() thor.Bytes32 { return engine.finalized.Load().(thor.Bytes32) } +func (engine *BFTEngine) Justified() thor.Bytes32 { + return engine.justified.Load().(thor.Bytes32) +} + // Accepts checks if the given block is on the same branch of finalized checkpoint. func (engine *BFTEngine) Accepts(parentID thor.Bytes32) (bool, error) { finalized := engine.Finalized() @@ -108,6 +126,25 @@ func (engine *BFTEngine) Select(header *block.Header) (bool, error) { return header.BetterThan(best), nil } +func (engine *BFTEngine) JustifyBlock(header *block.Header) error { + if getStorePoint(header.Number()) == header.Number() { + st, err := engine.computeState(header) + if err != nil { + return err + } + + if st.Justified { + // get last checkpoint + checkpoint, err := engine.repo.NewChain(header.ID()).GetBlockSummary(getCheckPoint(header.Number())) + if err != nil { + return err + } + engine.justified.Store(checkpoint.Header.ID()) + } + } + return nil +} + // CommitBlock commits bft state to storage. func (engine *BFTEngine) CommitBlock(header *block.Header, isPacking bool) error { // save quality and finalized at the end of each round @@ -223,7 +260,7 @@ func (engine *BFTEngine) ShouldVote(parentID thor.Bytes32) (bool, error) { return true, nil } -// computeState computes the bft state regarding the given block header. +// computeState computes the bft state regarding the given block header to the closest checkpoint. func (engine *BFTEngine) computeState(header *block.Header) (*bftState, error) { if cached, ok := engine.caches.state.Get(header.ID()); ok { return cached.(*bftState), nil diff --git a/bft/engine_test.go b/bft/engine_test.go index 3d2eb4a3f..10f232119 100644 --- a/bft/engine_test.go +++ b/bft/engine_test.go @@ -153,6 +153,10 @@ func (test *TestBFT) newBlock(parentSummary *chain.BlockSummary, master genesis. return nil, err } + if err = test.engine.JustifyBlock(b.Header()); err != nil { + return nil, err + } + if err = test.engine.CommitBlock(b.Header(), false); err != nil { return nil, err } @@ -222,6 +226,10 @@ func (test *TestBFT) pack(parentID thor.Bytes32, shouldVote bool, best bool) (*c return nil, err } + if err := test.engine.JustifyBlock(blk.Header); err != nil { + return nil, err + } + if err := test.engine.CommitBlock(blk.Header, true); err != nil { return nil, err } @@ -243,6 +251,7 @@ func TestNewEngine(t *testing.T) { genID := testBFT.repo.BestBlockSummary().Header.ID() assert.Equal(t, genID, testBFT.engine.Finalized()) + assert.Equal(t, genID, testBFT.engine.Justified()) } func TestNewBlock(t *testing.T) { @@ -394,6 +403,13 @@ func TestFinalized(t *testing.T) { } assert.Equal(t, finalized, testBFT.engine.Finalized()) + + justified, err := testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 2) + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, justified, testBFT.engine.Justified()) } func TestAccepts(t *testing.T) { diff --git a/cmd/thor/node/node.go b/cmd/thor/node/node.go index 663ff185f..ae0073add 100644 --- a/cmd/thor/node/node.go +++ b/cmd/thor/node/node.go @@ -365,6 +365,12 @@ func (n *Node) processBlock(newBlock *block.Block, stats *blockStats) (bool, err return errors.Wrap(err, "add block") } + if newBlock.Header().Number() >= n.forkConfig.FINALITY { + if err := n.bft.JustifyBlock(newBlock.Header()); err != nil { + return errors.Wrap(err, "bft justify") + } + } + // commit block in bft engine if newBlock.Header().Number() >= n.forkConfig.FINALITY { if err := n.bft.CommitBlock(newBlock.Header(), false); err != nil { diff --git a/cmd/thor/node/packer_loop.go b/cmd/thor/node/packer_loop.go index dfdc58b05..d4ffc3149 100644 --- a/cmd/thor/node/packer_loop.go +++ b/cmd/thor/node/packer_loop.go @@ -173,6 +173,12 @@ func (n *Node) pack(flow *packer.Flow) (err error) { return errors.Wrap(err, "add block") } + if newBlock.Header().Number() >= n.forkConfig.FINALITY { + if err := n.bft.JustifyBlock(newBlock.Header()); err != nil { + return errors.Wrap(err, "bft justify") + } + } + // commit block in bft engine if newBlock.Header().Number() >= n.forkConfig.FINALITY { if err := n.bft.CommitBlock(newBlock.Header(), true); err != nil { diff --git a/cmd/thor/solo/types.go b/cmd/thor/solo/types.go index a4f055122..56b0c0144 100644 --- a/cmd/thor/solo/types.go +++ b/cmd/thor/solo/types.go @@ -23,14 +23,20 @@ func (comm *Communicator) PeersStats() []*comm.PeerStats { // BFTEngine is a fake bft engine for solo. type BFTEngine struct { finalized thor.Bytes32 + justified thor.Bytes32 } func (engine *BFTEngine) Finalized() thor.Bytes32 { return engine.finalized } +func (engine *BFTEngine) Justified() thor.Bytes32 { + return engine.justified +} + func NewBFTEngine(repo *chain.Repository) *BFTEngine { return &BFTEngine{ finalized: repo.GenesisBlock().Header().ID(), + justified: repo.GenesisBlock().Header().ID(), } } From 36abe916dd21c9ad128d780bab0518f5abafa87c Mon Sep 17 00:00:00 2001 From: Paolo Galli Date: Fri, 26 Jul 2024 09:13:20 +0200 Subject: [PATCH 02/12] fix: missing err declaration --- api/accounts/accounts.go | 4 ++-- api/api.go | 2 +- api/blocks/blocks.go | 4 ++-- api/debug/debug.go | 4 ++-- api/utils/revisions.go | 4 ++-- bft/engine.go | 2 +- cmd/thor/node/packer_loop.go | 2 +- 7 files changed, 11 insertions(+), 11 deletions(-) diff --git a/api/accounts/accounts.go b/api/accounts/accounts.go index e8cd6cf90..2ef14ca87 100644 --- a/api/accounts/accounts.go +++ b/api/accounts/accounts.go @@ -31,7 +31,7 @@ type Accounts struct { stater *state.Stater callGasLimit uint64 forkConfig thor.ForkConfig - bft bft.Finalizer + bft bft.CommitLevel } func New( @@ -39,7 +39,7 @@ func New( stater *state.Stater, callGasLimit uint64, forkConfig thor.ForkConfig, - bft bft.Finalizer, + bft bft.CommitLevel, ) *Accounts { return &Accounts{ repo, diff --git a/api/api.go b/api/api.go index 0d929d294..9f425f66a 100644 --- a/api/api.go +++ b/api/api.go @@ -38,7 +38,7 @@ func New( stater *state.Stater, txPool *txpool.TxPool, logDB *logdb.LogDB, - bft bft.Finalizer, + bft bft.CommitLevel, nw node.Network, forkConfig thor.ForkConfig, allowedOrigins string, diff --git a/api/blocks/blocks.go b/api/blocks/blocks.go index 3062138c7..c3183a4c5 100644 --- a/api/blocks/blocks.go +++ b/api/blocks/blocks.go @@ -19,10 +19,10 @@ import ( type Blocks struct { repo *chain.Repository - bft bft.Finalizer + bft bft.CommitLevel } -func New(repo *chain.Repository, bft bft.Finalizer) *Blocks { +func New(repo *chain.Repository, bft bft.CommitLevel) *Blocks { return &Blocks{ repo, bft, diff --git a/api/debug/debug.go b/api/debug/debug.go index 646657c6f..f006be28b 100644 --- a/api/debug/debug.go +++ b/api/debug/debug.go @@ -44,10 +44,10 @@ type Debug struct { forkConfig thor.ForkConfig callGasLimit uint64 allowCustomTracer bool - bft bft.Finalizer + bft bft.CommitLevel } -func New(repo *chain.Repository, stater *state.Stater, forkConfig thor.ForkConfig, callGaslimit uint64, allowCustomTracer bool, bft bft.Finalizer) *Debug { +func New(repo *chain.Repository, stater *state.Stater, forkConfig thor.ForkConfig, callGaslimit uint64, allowCustomTracer bool, bft bft.CommitLevel) *Debug { return &Debug{ repo, stater, diff --git a/api/utils/revisions.go b/api/utils/revisions.go index 0b2b380e7..86ad78371 100644 --- a/api/utils/revisions.go +++ b/api/utils/revisions.go @@ -72,7 +72,7 @@ func ParseRevision(revision string, allowNext bool) (*Revision, error) { // GetSummary returns the block summary for the given revision, // revision required to be a deterministic block other than "next". -func GetSummary(rev *Revision, repo *chain.Repository, bft bft.Finalizer) (sum *chain.BlockSummary, err error) { +func GetSummary(rev *Revision, repo *chain.Repository, bft bft.CommitLevel) (sum *chain.BlockSummary, err error) { var id thor.Bytes32 switch rev := rev.val.(type) { case thor.Bytes32: @@ -104,7 +104,7 @@ func GetSummary(rev *Revision, repo *chain.Repository, bft bft.Finalizer) (sum * // GetSummaryAndState returns the block summary and state for the given revision, // this function supports the "next" revision. -func GetSummaryAndState(rev *Revision, repo *chain.Repository, bft bft.Finalizer, stater *state.Stater) (*chain.BlockSummary, *state.State, error) { +func GetSummaryAndState(rev *Revision, repo *chain.Repository, bft bft.CommitLevel, stater *state.Stater) (*chain.BlockSummary, *state.State, error) { if rev.IsNext() { best := repo.BestBlockSummary() diff --git a/bft/engine.go b/bft/engine.go index cff59e01d..566977b13 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -26,7 +26,7 @@ const dataStoreName = "bft.engine" var finalizedKey = []byte("finalized") var justifiedKey = []byte("justified") -type Finalizer interface { +type CommitLevel interface { Finalized() thor.Bytes32 Justified() thor.Bytes32 } diff --git a/cmd/thor/node/packer_loop.go b/cmd/thor/node/packer_loop.go index d4ffc3149..6272013cd 100644 --- a/cmd/thor/node/packer_loop.go +++ b/cmd/thor/node/packer_loop.go @@ -158,7 +158,7 @@ func (n *Node) pack(flow *packer.Flow) (err error) { // write logs if logEnabled { - if n.writeLogs(newBlock, receipts, oldBest.Header.ID()); err != nil { + if err := n.writeLogs(newBlock, receipts, oldBest.Header.ID()); err != nil { return errors.Wrap(err, "write logs") } } From a357938f39597cd0ceeb7d6ff877a31e7efff05f Mon Sep 17 00:00:00 2001 From: Paolo Galli Date: Fri, 26 Jul 2024 09:36:54 +0200 Subject: [PATCH 03/12] docs: add justified revision to thor.yaml --- api/doc/thor.yaml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/doc/thor.yaml b/api/doc/thor.yaml index 2c6d03d47..b629ff7b4 100644 --- a/api/doc/thor.yaml +++ b/api/doc/thor.yaml @@ -2252,7 +2252,7 @@ components: RevisionInQuery: name: revision in: query - description: Specify either `best`, `finalized`, a block number or block ID. If omitted, the `best` block is assumed. + description: Specify either `best`, `justified`, `finalized`, a block number or block ID. If omitted, the `best` block is assumed. schema: type: string @@ -2260,7 +2260,7 @@ components: name: revision in: query description: | - Specify either `best`,`next`, `finalized`, a block number or block ID. If omitted, the `best` block is assumed. + Specify either `best`, `next`, `justified`, `finalized`, a block number or block ID. If omitted, the `best` block is assumed. If the `next` block is specified, the call code will be executed on the next block, with the following: - The block number is the `best` block number plus one. @@ -2279,6 +2279,7 @@ components: - a block ID (hex string) - a block number (integer) - `best` stands for latest block + - `justified` stands for the justified block - `finalized` stands for the finalized block required: true schema: From cf472e3e0403b201bbaddf704efa80be761ccfc2 Mon Sep 17 00:00:00 2001 From: Paolo Galli Date: Fri, 26 Jul 2024 09:53:09 +0200 Subject: [PATCH 04/12] feat: storing justified blockId in engine.data --- bft/engine.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bft/engine.go b/bft/engine.go index 566977b13..faeca3acb 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -134,12 +134,15 @@ func (engine *BFTEngine) JustifyBlock(header *block.Header) error { } if st.Justified { - // get last checkpoint - checkpoint, err := engine.repo.NewChain(header.ID()).GetBlockSummary(getCheckPoint(header.Number())) + // get last checkpointId + checkpointId, err := engine.repo.NewChain(header.ID()).GetBlockID(getCheckPoint(header.Number())) if err != nil { return err } - engine.justified.Store(checkpoint.Header.ID()) + if err := engine.data.Put(justifiedKey, checkpointId[:]); err != nil { + return err + } + engine.justified.Store(checkpointId) } } return nil From 2a0d9b359b02ffb9de280ef24c85811aae4d315b Mon Sep 17 00:00:00 2001 From: tony Date: Mon, 5 Aug 2024 17:32:44 +0800 Subject: [PATCH 05/12] a non-persist way to implement jutified --- api/utils/revisions.go | 5 +- bft/engine.go | 97 +++++++++++++++++++++++------------- cmd/thor/node/node.go | 6 --- cmd/thor/node/packer_loop.go | 6 --- cmd/thor/solo/types.go | 4 +- 5 files changed, 67 insertions(+), 51 deletions(-) diff --git a/api/utils/revisions.go b/api/utils/revisions.go index 86ad78371..17bf7b450 100644 --- a/api/utils/revisions.go +++ b/api/utils/revisions.go @@ -89,7 +89,10 @@ func GetSummary(rev *Revision, repo *chain.Repository, bft bft.CommitLevel) (sum case revFinalized: id = bft.Finalized() case revJustified: - id = bft.Justified() + id, err = bft.Justified() + if err != nil { + return nil, err + } } } if id.IsZero() { diff --git a/bft/engine.go b/bft/engine.go index faeca3acb..e77caeea5 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -24,11 +24,15 @@ import ( const dataStoreName = "bft.engine" var finalizedKey = []byte("finalized") -var justifiedKey = []byte("justified") type CommitLevel interface { Finalized() thor.Bytes32 - Justified() thor.Bytes32 + Justified() (thor.Bytes32, error) +} + +type justified struct { + search thor.Bytes32 + value thor.Bytes32 } // BFTEngine tracks all votes of blocks, computes the finalized checkpoint. @@ -63,16 +67,6 @@ func NewEngine(repo *chain.Repository, mainDB *muxdb.MuxDB, forkConfig thor.Fork engine.caches.quality, _ = lru.New(16) engine.caches.justifier = cache.NewPrioCache(16) - // Restore justified block, if any - if val, err := engine.data.Get(justifiedKey); err != nil { - if !engine.data.IsNotFound(err) { - return nil, err - } - engine.justified.Store(engine.repo.GenesisBlock().Header().ID()) - } else { - engine.justified.Store(thor.BytesToBytes32(val)) - } - // Restore finalized block, if any if val, err := engine.data.Get(finalizedKey); err != nil { if !engine.data.IsNotFound(err) { @@ -91,8 +85,61 @@ func (engine *BFTEngine) Finalized() thor.Bytes32 { return engine.finalized.Load().(thor.Bytes32) } -func (engine *BFTEngine) Justified() thor.Bytes32 { - return engine.justified.Load().(thor.Bytes32) +func (engine *BFTEngine) Justified() (thor.Bytes32, error) { + finalized := engine.Finalized() + + if block.Number(finalized) == 0 { + return finalized, nil + } + + chain := engine.repo.NewBestChain() + headNumber := block.Number(chain.HeadID()) + + // current epoch is not concluded yet, so search from previous checkpoint + from := getCheckPoint(headNumber - thor.CheckpointInterval) + if getStorePoint(headNumber) == headNumber { + // current epoch is concluded + from = getCheckPoint(headNumber) + } + + fromId, err := chain.GetBlockID(from) + if err != nil { + return thor.Bytes32{}, err + } + + if val := engine.justified.Load(); val != nil && fromId == val.(justified).search { + return val.(justified).value, nil + } + + id := fromId + for { + quality, err := engine.getQuality(id) + if err != nil { + return thor.Bytes32{}, err + } + + parentID, err := chain.GetBlockID(block.Number(id) - thor.CheckpointInterval) + if err != nil { + return thor.Bytes32{}, err + } + + parentQuality, err := engine.getQuality(parentID) + if err != nil { + return thor.Bytes32{}, err + } + + if quality > parentQuality { + engine.justified.Store(justified{search: fromId, value: id}) + return id, nil + } + + if block.Number(parentID) <= block.Number(finalized) { + engine.justified.Store(justified{search: fromId, value: finalized}) + return finalized, nil + } + + id = parentID + } } // Accepts checks if the given block is on the same branch of finalized checkpoint. @@ -126,28 +173,6 @@ func (engine *BFTEngine) Select(header *block.Header) (bool, error) { return header.BetterThan(best), nil } -func (engine *BFTEngine) JustifyBlock(header *block.Header) error { - if getStorePoint(header.Number()) == header.Number() { - st, err := engine.computeState(header) - if err != nil { - return err - } - - if st.Justified { - // get last checkpointId - checkpointId, err := engine.repo.NewChain(header.ID()).GetBlockID(getCheckPoint(header.Number())) - if err != nil { - return err - } - if err := engine.data.Put(justifiedKey, checkpointId[:]); err != nil { - return err - } - engine.justified.Store(checkpointId) - } - } - return nil -} - // CommitBlock commits bft state to storage. func (engine *BFTEngine) CommitBlock(header *block.Header, isPacking bool) error { // save quality and finalized at the end of each round diff --git a/cmd/thor/node/node.go b/cmd/thor/node/node.go index ae0073add..663ff185f 100644 --- a/cmd/thor/node/node.go +++ b/cmd/thor/node/node.go @@ -365,12 +365,6 @@ func (n *Node) processBlock(newBlock *block.Block, stats *blockStats) (bool, err return errors.Wrap(err, "add block") } - if newBlock.Header().Number() >= n.forkConfig.FINALITY { - if err := n.bft.JustifyBlock(newBlock.Header()); err != nil { - return errors.Wrap(err, "bft justify") - } - } - // commit block in bft engine if newBlock.Header().Number() >= n.forkConfig.FINALITY { if err := n.bft.CommitBlock(newBlock.Header(), false); err != nil { diff --git a/cmd/thor/node/packer_loop.go b/cmd/thor/node/packer_loop.go index 6272013cd..44e7f1ded 100644 --- a/cmd/thor/node/packer_loop.go +++ b/cmd/thor/node/packer_loop.go @@ -173,12 +173,6 @@ func (n *Node) pack(flow *packer.Flow) (err error) { return errors.Wrap(err, "add block") } - if newBlock.Header().Number() >= n.forkConfig.FINALITY { - if err := n.bft.JustifyBlock(newBlock.Header()); err != nil { - return errors.Wrap(err, "bft justify") - } - } - // commit block in bft engine if newBlock.Header().Number() >= n.forkConfig.FINALITY { if err := n.bft.CommitBlock(newBlock.Header(), true); err != nil { diff --git a/cmd/thor/solo/types.go b/cmd/thor/solo/types.go index 56b0c0144..c431ece0d 100644 --- a/cmd/thor/solo/types.go +++ b/cmd/thor/solo/types.go @@ -30,8 +30,8 @@ func (engine *BFTEngine) Finalized() thor.Bytes32 { return engine.finalized } -func (engine *BFTEngine) Justified() thor.Bytes32 { - return engine.justified +func (engine *BFTEngine) Justified() (thor.Bytes32, error) { + return engine.justified, nil } func NewBFTEngine(repo *chain.Repository) *BFTEngine { From ea7fd2a7ea8dcae9cd80f8b73002f19d4c26cfb6 Mon Sep 17 00:00:00 2001 From: tony Date: Tue, 6 Aug 2024 11:13:43 +0800 Subject: [PATCH 06/12] use store point to restore quality --- bft/engine.go | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/bft/engine.go b/bft/engine.go index e77caeea5..cde354c43 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -95,11 +95,10 @@ func (engine *BFTEngine) Justified() (thor.Bytes32, error) { chain := engine.repo.NewBestChain() headNumber := block.Number(chain.HeadID()) - // current epoch is not concluded yet, so search from previous checkpoint - from := getCheckPoint(headNumber - thor.CheckpointInterval) - if getStorePoint(headNumber) == headNumber { - // current epoch is concluded - from = getCheckPoint(headNumber) + from := getStorePoint(headNumber) + if from > headNumber { + // current epoch is not concluded yet + from -= thor.CheckpointInterval } fromId, err := chain.GetBlockID(from) @@ -129,13 +128,18 @@ func (engine *BFTEngine) Justified() (thor.Bytes32, error) { } if quality > parentQuality { - engine.justified.Store(justified{search: fromId, value: id}) - return id, nil + checkpoint, err := chain.GetBlockID(getCheckPoint(block.Number(id))) + if err != nil { + return thor.Bytes32{}, err + } + + engine.justified.Store(justified{search: fromId, value: checkpoint}) + return checkpoint, nil } - if block.Number(parentID) <= block.Number(finalized) { - engine.justified.Store(justified{search: fromId, value: finalized}) - return finalized, nil + if getCheckPoint(block.Number(parentID)) <= block.Number(finalized) { + // this should never happen, if it does, something wrong with the storage + return thor.Bytes32{}, errors.New("failed to find the justified checkpoint") } id = parentID From 07d0cee796aea36f273574b8ed6274f115d0c80f Mon Sep 17 00:00:00 2001 From: Paolo Galli Date: Tue, 6 Aug 2024 10:05:04 +0200 Subject: [PATCH 07/12] refactor: rename CommitLevel interface --- api/accounts/accounts.go | 4 ++-- api/api.go | 2 +- api/blocks/blocks.go | 4 ++-- api/debug/debug.go | 4 ++-- api/utils/revisions.go | 4 ++-- bft/engine.go | 2 +- 6 files changed, 10 insertions(+), 10 deletions(-) diff --git a/api/accounts/accounts.go b/api/accounts/accounts.go index 2ef14ca87..a6d1df3ee 100644 --- a/api/accounts/accounts.go +++ b/api/accounts/accounts.go @@ -31,7 +31,7 @@ type Accounts struct { stater *state.Stater callGasLimit uint64 forkConfig thor.ForkConfig - bft bft.CommitLevel + bft bft.Committer } func New( @@ -39,7 +39,7 @@ func New( stater *state.Stater, callGasLimit uint64, forkConfig thor.ForkConfig, - bft bft.CommitLevel, + bft bft.Committer, ) *Accounts { return &Accounts{ repo, diff --git a/api/api.go b/api/api.go index 9f425f66a..c63573764 100644 --- a/api/api.go +++ b/api/api.go @@ -38,7 +38,7 @@ func New( stater *state.Stater, txPool *txpool.TxPool, logDB *logdb.LogDB, - bft bft.CommitLevel, + bft bft.Committer, nw node.Network, forkConfig thor.ForkConfig, allowedOrigins string, diff --git a/api/blocks/blocks.go b/api/blocks/blocks.go index c3183a4c5..bddb3ac12 100644 --- a/api/blocks/blocks.go +++ b/api/blocks/blocks.go @@ -19,10 +19,10 @@ import ( type Blocks struct { repo *chain.Repository - bft bft.CommitLevel + bft bft.Committer } -func New(repo *chain.Repository, bft bft.CommitLevel) *Blocks { +func New(repo *chain.Repository, bft bft.Committer) *Blocks { return &Blocks{ repo, bft, diff --git a/api/debug/debug.go b/api/debug/debug.go index f006be28b..bcdb32950 100644 --- a/api/debug/debug.go +++ b/api/debug/debug.go @@ -44,10 +44,10 @@ type Debug struct { forkConfig thor.ForkConfig callGasLimit uint64 allowCustomTracer bool - bft bft.CommitLevel + bft bft.Committer } -func New(repo *chain.Repository, stater *state.Stater, forkConfig thor.ForkConfig, callGaslimit uint64, allowCustomTracer bool, bft bft.CommitLevel) *Debug { +func New(repo *chain.Repository, stater *state.Stater, forkConfig thor.ForkConfig, callGaslimit uint64, allowCustomTracer bool, bft bft.Committer) *Debug { return &Debug{ repo, stater, diff --git a/api/utils/revisions.go b/api/utils/revisions.go index 86ad78371..61675aebe 100644 --- a/api/utils/revisions.go +++ b/api/utils/revisions.go @@ -72,7 +72,7 @@ func ParseRevision(revision string, allowNext bool) (*Revision, error) { // GetSummary returns the block summary for the given revision, // revision required to be a deterministic block other than "next". -func GetSummary(rev *Revision, repo *chain.Repository, bft bft.CommitLevel) (sum *chain.BlockSummary, err error) { +func GetSummary(rev *Revision, repo *chain.Repository, bft bft.Committer) (sum *chain.BlockSummary, err error) { var id thor.Bytes32 switch rev := rev.val.(type) { case thor.Bytes32: @@ -104,7 +104,7 @@ func GetSummary(rev *Revision, repo *chain.Repository, bft bft.CommitLevel) (sum // GetSummaryAndState returns the block summary and state for the given revision, // this function supports the "next" revision. -func GetSummaryAndState(rev *Revision, repo *chain.Repository, bft bft.CommitLevel, stater *state.Stater) (*chain.BlockSummary, *state.State, error) { +func GetSummaryAndState(rev *Revision, repo *chain.Repository, bft bft.Committer, stater *state.Stater) (*chain.BlockSummary, *state.State, error) { if rev.IsNext() { best := repo.BestBlockSummary() diff --git a/bft/engine.go b/bft/engine.go index faeca3acb..a89dc399b 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -26,7 +26,7 @@ const dataStoreName = "bft.engine" var finalizedKey = []byte("finalized") var justifiedKey = []byte("justified") -type CommitLevel interface { +type Committer interface { Finalized() thor.Bytes32 Justified() thor.Bytes32 } From 6479ec32d1b4f0d4ba5e2c44ee59304dd7fa6cd4 Mon Sep 17 00:00:00 2001 From: Paolo Galli Date: Tue, 6 Aug 2024 15:16:25 +0200 Subject: [PATCH 08/12] refactor: change approach to non-persist the justified block --- bft/engine_test.go | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/bft/engine_test.go b/bft/engine_test.go index 10f232119..353cb810d 100644 --- a/bft/engine_test.go +++ b/bft/engine_test.go @@ -153,10 +153,6 @@ func (test *TestBFT) newBlock(parentSummary *chain.BlockSummary, master genesis. return nil, err } - if err = test.engine.JustifyBlock(b.Header()); err != nil { - return nil, err - } - if err = test.engine.CommitBlock(b.Header(), false); err != nil { return nil, err } @@ -226,10 +222,6 @@ func (test *TestBFT) pack(parentID thor.Bytes32, shouldVote bool, best bool) (*c return nil, err } - if err := test.engine.JustifyBlock(blk.Header); err != nil { - return nil, err - } - if err := test.engine.CommitBlock(blk.Header, true); err != nil { return nil, err } @@ -251,7 +243,10 @@ func TestNewEngine(t *testing.T) { genID := testBFT.repo.BestBlockSummary().Header.ID() assert.Equal(t, genID, testBFT.engine.Finalized()) - assert.Equal(t, genID, testBFT.engine.Justified()) + + j, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, genID, j) } func TestNewBlock(t *testing.T) { @@ -409,7 +404,9 @@ func TestFinalized(t *testing.T) { t.Fatal(err) } - assert.Equal(t, justified, testBFT.engine.Justified()) + j, err := testBFT.engine.Justified() + assert.NoError(t, err) + assert.Equal(t, justified, j) } func TestAccepts(t *testing.T) { From 0784024ffc71e7e7efc3cca7413c31b975e820bd Mon Sep 17 00:00:00 2001 From: tony Date: Thu, 8 Aug 2024 15:36:49 +0800 Subject: [PATCH 09/12] bft: add justified tests --- bft/engine_test.go | 157 +++++++++++++++++++++++++++++++---------- test/datagen/addess.go | 19 +++++ test/datagen/hash.go | 19 +++++ 3 files changed, 157 insertions(+), 38 deletions(-) create mode 100644 test/datagen/addess.go create mode 100644 test/datagen/hash.go diff --git a/bft/engine_test.go b/bft/engine_test.go index 353cb810d..71606150d 100644 --- a/bft/engine_test.go +++ b/bft/engine_test.go @@ -5,7 +5,6 @@ package bft import ( - "crypto/rand" "math" "testing" @@ -16,6 +15,7 @@ import ( "github.com/vechain/thor/v2/muxdb" "github.com/vechain/thor/v2/packer" "github.com/vechain/thor/v2/state" + "github.com/vechain/thor/v2/test/datagen" "github.com/vechain/thor/v2/thor" ) @@ -39,20 +39,6 @@ var defaultFC = thor.ForkConfig{ FINALITY: 0, } -func RandomAddress() thor.Address { - var addr thor.Address - - rand.Read(addr[:]) - return addr -} - -func RandomBytes32() thor.Bytes32 { - var b32 thor.Bytes32 - - rand.Read(b32[:]) - return b32 -} - func newTestBft(forkCfg thor.ForkConfig) (*TestBFT, error) { db := muxdb.NewMem() @@ -399,14 +385,15 @@ func TestFinalized(t *testing.T) { assert.Equal(t, finalized, testBFT.engine.Finalized()) - justified, err := testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 2) + jc, err := testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 2) if err != nil { t.Fatal(err) } j, err := testBFT.engine.Justified() assert.NoError(t, err) - assert.Equal(t, justified, j) + assert.Equal(t, jc, j) + assert.Equal(t, jc, testBFT.engine.justified.Load().(justified).value) } func TestAccepts(t *testing.T) { @@ -726,8 +713,8 @@ func TestJustifier(t *testing.T) { var blkID thor.Bytes32 for i := 0; i <= MaxBlockProposers*2/3; i++ { - blkID = RandomBytes32() - vs.AddBlock(blkID, RandomAddress(), true) + blkID = datagen.RandomHash() + vs.AddBlock(blkID, datagen.RandomAddress(), true) } st := vs.Summarize() @@ -736,7 +723,7 @@ func TestJustifier(t *testing.T) { assert.True(t, st.Committed) // add vote after commits，commit/justify stays the same - vs.AddBlock(RandomBytes32(), RandomAddress(), true) + vs.AddBlock(datagen.RandomHash(), datagen.RandomAddress(), true) st = vs.Summarize() assert.Equal(t, uint32(3), st.Quality) assert.True(t, st.Justified) @@ -759,8 +746,8 @@ func TestJustifier(t *testing.T) { var blkID thor.Bytes32 for i := 0; i <= MaxBlockProposers*2/3; i++ { - blkID = RandomBytes32() - vs.AddBlock(blkID, RandomAddress(), false) + blkID = datagen.RandomHash() + vs.AddBlock(blkID, datagen.RandomAddress(), false) } st := vs.Summarize() @@ -786,13 +773,13 @@ func TestJustifier(t *testing.T) { var blkID thor.Bytes32 // vote times COM for i := 0; i < MaxBlockProposers*2/3; i++ { - blkID = RandomBytes32() - vs.AddBlock(blkID, RandomAddress(), true) + blkID = datagen.RandomHash() + vs.AddBlock(blkID, datagen.RandomAddress(), true) } - master := RandomAddress() + master := datagen.RandomAddress() // master votes WIT - blkID = RandomBytes32() + blkID = datagen.RandomHash() vs.AddBlock(blkID, master, false) // justifies but not committed @@ -800,7 +787,7 @@ func TestJustifier(t *testing.T) { assert.True(t, st.Justified) assert.False(t, st.Committed) - blkID = RandomBytes32() + blkID = datagen.RandomHash() // master votes COM vs.AddBlock(blkID, master, true) @@ -809,8 +796,8 @@ func TestJustifier(t *testing.T) { assert.False(t, st.Committed) // another master votes WIT - blkID = RandomBytes32() - vs.AddBlock(blkID, RandomAddress(), true) + blkID = datagen.RandomHash() + vs.AddBlock(blkID, datagen.RandomAddress(), true) st = vs.Summarize() assert.True(t, st.Committed) }, @@ -826,20 +813,20 @@ func TestJustifier(t *testing.T) { t.Fatal(err) } - master := RandomAddress() - vs.AddBlock(RandomBytes32(), master, true) + master := datagen.RandomAddress() + vs.AddBlock(datagen.RandomHash(), master, true) assert.Equal(t, true, vs.votes[master]) assert.Equal(t, uint64(1), vs.comVotes) - vs.AddBlock(RandomBytes32(), master, false) + vs.AddBlock(datagen.RandomHash(), master, false) assert.Equal(t, false, vs.votes[master]) assert.Equal(t, uint64(0), vs.comVotes) - vs.AddBlock(RandomBytes32(), master, true) + vs.AddBlock(datagen.RandomHash(), master, true) assert.Equal(t, false, vs.votes[master]) assert.Equal(t, uint64(0), vs.comVotes) - vs.AddBlock(RandomBytes32(), master, false) + vs.AddBlock(datagen.RandomHash(), master, false) assert.Equal(t, false, vs.votes[master]) assert.Equal(t, uint64(0), vs.comVotes) @@ -847,19 +834,19 @@ func TestJustifier(t *testing.T) { if err != nil { t.Fatal(err) } - vs.AddBlock(RandomBytes32(), master, false) + vs.AddBlock(datagen.RandomHash(), master, false) assert.Equal(t, false, vs.votes[master]) assert.Equal(t, uint64(0), vs.comVotes) - vs.AddBlock(RandomBytes32(), master, true) + vs.AddBlock(datagen.RandomHash(), master, true) assert.Equal(t, false, vs.votes[master]) assert.Equal(t, uint64(0), vs.comVotes) - vs.AddBlock(RandomBytes32(), master, true) + vs.AddBlock(datagen.RandomHash(), master, true) assert.Equal(t, false, vs.votes[master]) assert.Equal(t, uint64(0), vs.comVotes) - vs.AddBlock(RandomBytes32(), master, false) + vs.AddBlock(datagen.RandomHash(), master, false) assert.Equal(t, false, vs.votes[master]) assert.Equal(t, uint64(0), vs.comVotes) }, @@ -871,3 +858,97 @@ func TestJustifier(t *testing.T) { }) } } + +func TestJustified(t *testing.T) { + testBFT, err := newTestBft(defaultFC) + if err != nil { + t.Fatal(err) + } + + if err = testBFT.fastForward(thor.CheckpointInterval*3 - 1); err != nil { + t.Fatal(err) + } + + // chain stops the end of third bft round,should commit the second checkpoint + finalized, err := testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval) + if err != nil { + t.Fatal(err) + } + + assert.Equal(t, finalized, testBFT.engine.Finalized()) + + jc, err := testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 2) + if err != nil { + t.Fatal(err) + } + + actual, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, jc, actual) + assert.Equal(t, jc, testBFT.engine.justified.Load().(justified).value) + again, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, jc, again) + assert.Equal(t, jc, testBFT.engine.justified.Load().(justified).value) + + if err = testBFT.fastForwardWithMinority(thor.CheckpointInterval); err != nil { + t.Fatal(err) + } + + // move forward with minority, should not change finalized and justified + assert.Equal(t, finalized, testBFT.engine.Finalized()) + actual, err = testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, jc, actual) + + if err = testBFT.fastForwardWithMinority(thor.CheckpointInterval); err != nil { + t.Fatal(err) + } + + // move forward with minority, should not change finalized and justified + assert.Equal(t, finalized, testBFT.engine.Finalized()) + actual, err = testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, jc, actual) + + // move forward with majority + if err = testBFT.fastForward(thor.CheckpointInterval); err != nil { + t.Fatal(err) + } + + // finalized should move forward 1 epoch + finalized, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 2) + if err != nil { + t.Fatal(err) + } + assert.Equal(t, finalized, testBFT.engine.Finalized()) + + jc, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 5) + if err != nil { + t.Fatal(err) + } + actual, err = testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, jc, actual) + + // move forward with majority + if err = testBFT.fastForward(thor.CheckpointInterval); err != nil { + t.Fatal(err) + } + + // finalized should be head(concluded) - 2 epoch + finalized, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 5) + if err != nil { + t.Fatal(err) + } + assert.Equal(t, finalized, testBFT.engine.Finalized()) + + // justified should be head(concluded) -1 epoch + jc, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 6) + if err != nil { + t.Fatal(err) + } + actual, err = testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, jc, actual) +} diff --git a/test/datagen/addess.go b/test/datagen/addess.go new file mode 100644 index 000000000..882159f6e --- /dev/null +++ b/test/datagen/addess.go @@ -0,0 +1,19 @@ +// Copyright (c) 2024 The VeChainThor developers + +// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying +// file LICENSE or + +package datagen + +import ( + "crypto/rand" + + "github.com/vechain/thor/v2/thor" +) + +func RandomAddress() thor.Address { + var addr thor.Address + + rand.Read(addr[:]) + return addr +} diff --git a/test/datagen/hash.go b/test/datagen/hash.go new file mode 100644 index 000000000..2b15709f0 --- /dev/null +++ b/test/datagen/hash.go @@ -0,0 +1,19 @@ +// Copyright (c) 2024 The VeChainThor developers + +// Distributed under the GNU Lesser General Public License v3.0 software license, see the accompanying +// file LICENSE or + +package datagen + +import ( + "crypto/rand" + + "github.com/vechain/thor/v2/thor" +) + +func RandomHash() thor.Bytes32 { + var b32 thor.Bytes32 + + rand.Read(b32[:]) + return b32 +} From c661c6180acef6306b462e68b0716ac36ee2d985 Mon Sep 17 00:00:00 2001 From: Paolo Galli Date: Fri, 9 Aug 2024 16:35:31 +0200 Subject: [PATCH 10/12] fix: rename file name and simplified error check in test --- api/blocks/blocks_test.go | 5 ++--- test/datagen/{addess.go => address.go} | 0 2 files changed, 2 insertions(+), 3 deletions(-) rename test/datagen/{addess.go => address.go} (100%) diff --git a/api/blocks/blocks_test.go b/api/blocks/blocks_test.go index d7cd786ca..d8ef60771 100644 --- a/api/blocks/blocks_test.go +++ b/api/blocks/blocks_test.go @@ -20,6 +20,7 @@ import ( "github.com/ethereum/go-ethereum/crypto" "github.com/gorilla/mux" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/vechain/thor/v2/api/blocks" "github.com/vechain/thor/v2/block" "github.com/vechain/thor/v2/chain" @@ -103,9 +104,7 @@ func testGetFinalizedBlock(t *testing.T) { func testGetJustifiedBlock(t *testing.T) { res, statusCode := httpGet(t, ts.URL+"/blocks/justified") justified := new(blocks.JSONCollapsedBlock) - if err := json.Unmarshal(res, &justified); err != nil { - t.Fatal(err) - } + require.NoError(t, json.Unmarshal(res, &justified)) assert.Equal(t, http.StatusOK, statusCode) assert.Equal(t, uint32(0), justified.Number) diff --git a/test/datagen/addess.go b/test/datagen/address.go similarity index 100% rename from test/datagen/addess.go rename to test/datagen/address.go From 2c5b44287461328f017d6d091ac32ed4c0901b50 Mon Sep 17 00:00:00 2001 From: tony Date: Mon, 19 Aug 2024 12:03:34 +0800 Subject: [PATCH 11/12] improve bft package --- bft/engine.go | 88 +++++++-------- bft/engine_test.go | 268 ++++++++++++++++++++++++++++++--------------- 2 files changed, 222 insertions(+), 134 deletions(-) diff --git a/bft/engine.go b/bft/engine.go index 250f697d5..60762523d 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -85,65 +85,50 @@ func (engine *BFTEngine) Finalized() thor.Bytes32 { return engine.finalized.Load().(thor.Bytes32) } +// Justified returns the justified checkpoint. func (engine *BFTEngine) Justified() (thor.Bytes32, error) { + head := engine.repo.BestBlockSummary().Header finalized := engine.Finalized() - if block.Number(finalized) == 0 { + // if head is in the first epoch and not concluded yet + if head.Number() < getCheckPoint(engine.forkConfig.FINALITY)+thor.CheckpointInterval-1 { return finalized, nil } - chain := engine.repo.NewBestChain() - headNumber := block.Number(chain.HeadID()) - - from := getStorePoint(headNumber) - if from > headNumber { - // current epoch is not concluded yet - from -= thor.CheckpointInterval + // find the recent concluded checkpoint + concluded := getCheckPoint(head.Number()) + if head.Number() < getStorePoint(head.Number()) { + concluded -= thor.CheckpointInterval } - fromId, err := chain.GetBlockID(from) + headChain := engine.repo.NewChain(head.ID()) + + // storeID is the block id where an epoch concluded + storeID, err := headChain.GetBlockID(getStorePoint(concluded)) if err != nil { return thor.Bytes32{}, err } - if val := engine.justified.Load(); val != nil && fromId == val.(justified).search { + if val := engine.justified.Load(); val != nil && storeID == val.(justified).search { return val.(justified).value, nil } - id := fromId - for { - quality, err := engine.getQuality(id) - if err != nil { - return thor.Bytes32{}, err - } - - parentID, err := chain.GetBlockID(block.Number(id) - thor.CheckpointInterval) - if err != nil { - return thor.Bytes32{}, err - } - - parentQuality, err := engine.getQuality(parentID) - if err != nil { - return thor.Bytes32{}, err - } - - if quality > parentQuality { - checkpoint, err := chain.GetBlockID(getCheckPoint(block.Number(id))) - if err != nil { - return thor.Bytes32{}, err - } - - engine.justified.Store(justified{search: fromId, value: checkpoint}) - return checkpoint, nil - } + quality, err := engine.getQuality(storeID) + if err != nil { + return thor.Bytes32{}, err + } - if getCheckPoint(block.Number(parentID)) <= block.Number(finalized) { - // this should never happen, if it does, something wrong with the storage - return thor.Bytes32{}, errors.New("failed to find the justified checkpoint") - } + if quality == 0 { + return finalized, nil + } - id = parentID + checkpoint, err := engine.findCheckpointByQuality(quality, finalized, storeID) + if err != nil { + return thor.Bytes32{}, err } + + engine.justified.Store(justified{search: storeID, value: checkpoint}) + return checkpoint, nil } // Accepts checks if the given block is on the same branch of finalized checkpoint. @@ -192,7 +177,7 @@ func (engine *BFTEngine) CommitBlock(header *block.Header, isPacking bool) error engine.caches.quality.Add(header.ID(), state.Quality) if state.Committed && state.Quality > 1 { - id, err := engine.findCheckpointByQuality(state.Quality-1, engine.Finalized(), header.ParentID()) + id, err := engine.findCheckpointByQuality(state.Quality-1, engine.Finalized(), header.ID()) if err != nil { return err } @@ -251,17 +236,23 @@ func (engine *BFTEngine) ShouldVote(parentID thor.Bytes32) (bool, error) { headQuality := st.Quality finalized := engine.Finalized() + chain := engine.repo.NewChain(parentID) // most recent justified checkpoint var recentJC thor.Bytes32 if st.Justified { // if justified in this round, use this round's checkpoint - checkpoint, err := engine.repo.NewChain(parentID).GetBlockID(getCheckPoint(block.Number(parentID))) + checkpoint, err := chain.GetBlockID(getCheckPoint(block.Number(parentID))) if err != nil { return false, err } recentJC = checkpoint } else { - checkpoint, err := engine.findCheckpointByQuality(headQuality, finalized, parentID) + // if current round is not justified, find the most recent justified checkpoint + prev, err := chain.GetBlockID(getStorePoint(block.Number(parentID) - thor.CheckpointInterval)) + if err != nil { + return false, err + } + checkpoint, err := engine.findCheckpointByQuality(headQuality, finalized, prev) if err != nil { return false, err } @@ -347,7 +338,8 @@ func (engine *BFTEngine) computeState(header *block.Header) (*bftState, error) { } // findCheckpointByQuality finds the first checkpoint reaches the given quality. -func (engine *BFTEngine) findCheckpointByQuality(target uint32, finalized, parentID thor.Bytes32) (blockID thor.Bytes32, err error) { +// It is caller's responsibility to ensure the epoch that headID belongs to is concluded. +func (engine *BFTEngine) findCheckpointByQuality(target uint32, finalized, headID thor.Bytes32) (blockID thor.Bytes32, err error) { defer func() { if e := recover(); e != nil { err = e.(error) @@ -360,7 +352,7 @@ func (engine *BFTEngine) findCheckpointByQuality(target uint32, finalized, paren searchStart = getCheckPoint(engine.forkConfig.FINALITY) } - c := engine.repo.NewChain(parentID) + c := engine.repo.NewChain(headID) get := func(i int) (uint32, error) { id, err := c.GetBlockID(getStorePoint(searchStart + uint32(i)*thor.CheckpointInterval)) if err != nil { @@ -369,7 +361,8 @@ func (engine *BFTEngine) findCheckpointByQuality(target uint32, finalized, paren return engine.getQuality(id) } - n := int((block.Number(parentID) + 1 - searchStart) / thor.CheckpointInterval) + // sort.Search searches from [0, n) + n := int((block.Number(headID)-searchStart)/thor.CheckpointInterval) + 1 num := sort.Search(n, func(i int) bool { quality, err := get(i) if err != nil { @@ -379,6 +372,7 @@ func (engine *BFTEngine) findCheckpointByQuality(target uint32, finalized, paren return quality >= target }) + // n means not found for sort.Search if num == n { return thor.Bytes32{}, errors.New("failed find the block by quality") } diff --git a/bft/engine_test.go b/bft/engine_test.go index 71606150d..2480bbf79 100644 --- a/bft/engine_test.go +++ b/bft/engine_test.go @@ -5,11 +5,11 @@ package bft import ( - "math" "testing" "github.com/ethereum/go-ethereum/crypto" "github.com/stretchr/testify/assert" + "github.com/vechain/thor/v2/block" "github.com/vechain/thor/v2/chain" "github.com/vechain/thor/v2/genesis" "github.com/vechain/thor/v2/muxdb" @@ -29,14 +29,13 @@ type TestBFT struct { const MaxBlockProposers = 11 -var devAccounts = genesis.DevAccounts() -var defaultFC = thor.ForkConfig{ - VIP191: math.MaxUint32, - ETH_CONST: math.MaxUint32, - BLOCKLIST: math.MaxUint32, - ETH_IST: math.MaxUint32, - VIP214: math.MaxUint32, - FINALITY: 0, +var ( + devAccounts = genesis.DevAccounts() + defaultFC = thor.NoFork +) + +func init() { + defaultFC.FINALITY = 0 } func newTestBft(forkCfg thor.ForkConfig) (*TestBFT, error) { @@ -139,8 +138,10 @@ func (test *TestBFT) newBlock(parentSummary *chain.BlockSummary, master genesis. return nil, err } - if err = test.engine.CommitBlock(b.Header(), false); err != nil { - return nil, err + if b.Header().Number() >= test.fc.FINALITY { + if err = test.engine.CommitBlock(b.Header(), false); err != nil { + return nil, err + } } return test.repo.GetBlockSummary(b.Header().ID()) @@ -208,8 +209,10 @@ func (test *TestBFT) pack(parentID thor.Bytes32, shouldVote bool, best bool) (*c return nil, err } - if err := test.engine.CommitBlock(blk.Header, true); err != nil { - return nil, err + if blk.Header.Number() >= test.fc.FINALITY { + if err := test.engine.CommitBlock(blk.Header, true); err != nil { + return nil, err + } } if best { @@ -629,6 +632,20 @@ func TestGetVote(t *testing.T) { assert.Equal(t, false, v) }, + }, { + "test findCheckpointByQuality edge case, should not fail", func(t *testing.T) { + testBFT, err := newTestBft(defaultFC) + if err != nil { + t.Fatal(err) + } + + testBFT.fastForwardWithMinority(thor.CheckpointInterval * 3) + testBFT.fastForward(thor.CheckpointInterval*1 + 3) + _, err = testBFT.engine.ShouldVote(testBFT.repo.BestBlockSummary().Header.ID()) + if err != nil { + t.Fatal(err) + } + }, }, } @@ -860,95 +877,172 @@ func TestJustifier(t *testing.T) { } func TestJustified(t *testing.T) { - testBFT, err := newTestBft(defaultFC) - if err != nil { - t.Fatal(err) - } + tests := []struct { + name string + testFunc func(*testing.T) + }{ + { + "first several rounds, never justified", func(t *testing.T) { + testBFT, err := newTestBft(defaultFC) + if err != nil { + t.Fatal(err) + } - if err = testBFT.fastForward(thor.CheckpointInterval*3 - 1); err != nil { - t.Fatal(err) - } + for i := 0; i < 3*thor.CheckpointInterval; i++ { + if err = testBFT.fastForwardWithMinority(1); err != nil { + t.Fatal(err) + } - // chain stops the end of third bft round,should commit the second checkpoint - finalized, err := testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval) - if err != nil { - t.Fatal(err) - } + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), justified) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), testBFT.engine.Finalized()) + } + }, + }, { + "first several rounds, get justified", func(t *testing.T) { + testBFT, err := newTestBft(defaultFC) + if err != nil { + t.Fatal(err) + } - assert.Equal(t, finalized, testBFT.engine.Finalized()) + for i := 0; i < 2*thor.CheckpointInterval-2; i++ { + if err = testBFT.fastForward(1); err != nil { + t.Fatal(err) + } - jc, err := testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 2) - if err != nil { - t.Fatal(err) - } + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), justified) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), testBFT.engine.Finalized()) + } - actual, err := testBFT.engine.Justified() - assert.Nil(t, err) - assert.Equal(t, jc, actual) - assert.Equal(t, jc, testBFT.engine.justified.Load().(justified).value) - again, err := testBFT.engine.Justified() - assert.Nil(t, err) - assert.Equal(t, jc, again) - assert.Equal(t, jc, testBFT.engine.justified.Load().(justified).value) + if err = testBFT.fastForward(1); err != nil { + t.Fatal(err) + } + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, uint32(thor.CheckpointInterval), block.Number(justified)) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), testBFT.engine.Finalized()) + }, + }, { + "first three not justified rounds, then justified", func(t *testing.T) { + testBFT, err := newTestBft(defaultFC) + if err != nil { + t.Fatal(err) + } - if err = testBFT.fastForwardWithMinority(thor.CheckpointInterval); err != nil { - t.Fatal(err) - } + if err = testBFT.fastForwardWithMinority(3*thor.CheckpointInterval - 1); err != nil { + t.Fatal(err) + } - // move forward with minority, should not change finalized and justified - assert.Equal(t, finalized, testBFT.engine.Finalized()) - actual, err = testBFT.engine.Justified() - assert.Nil(t, err) - assert.Equal(t, jc, actual) + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), justified) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), testBFT.engine.Finalized()) - if err = testBFT.fastForwardWithMinority(thor.CheckpointInterval); err != nil { - t.Fatal(err) - } + if err = testBFT.fastForward(thor.CheckpointInterval); err != nil { + t.Fatal(err) + } + justified, err = testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, uint32(3*thor.CheckpointInterval), block.Number(justified)) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), testBFT.engine.Finalized()) + }, + }, { + "get finalized, then justified", func(t *testing.T) { + testBFT, err := newTestBft(defaultFC) + if err != nil { + t.Fatal(err) + } - // move forward with minority, should not change finalized and justified - assert.Equal(t, finalized, testBFT.engine.Finalized()) - actual, err = testBFT.engine.Justified() - assert.Nil(t, err) - assert.Equal(t, jc, actual) + if err = testBFT.fastForward(3*thor.CheckpointInterval - 1); err != nil { + t.Fatal(err) + } - // move forward with majority - if err = testBFT.fastForward(thor.CheckpointInterval); err != nil { - t.Fatal(err) - } + assert.Equal(t, uint32(thor.CheckpointInterval), block.Number(testBFT.engine.Finalized())) - // finalized should move forward 1 epoch - finalized, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 2) - if err != nil { - t.Fatal(err) - } - assert.Equal(t, finalized, testBFT.engine.Finalized()) + if err = testBFT.fastForward(thor.CheckpointInterval - 1); err != nil { + t.Fatal(err) + } - jc, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 5) - if err != nil { - t.Fatal(err) - } - actual, err = testBFT.engine.Justified() - assert.Nil(t, err) - assert.Equal(t, jc, actual) + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + // current epoch is not concluded + assert.Equal(t, uint32(2*thor.CheckpointInterval), block.Number(justified)) - // move forward with majority - if err = testBFT.fastForward(thor.CheckpointInterval); err != nil { - t.Fatal(err) - } + if err = testBFT.fastForward(1); err != nil { + t.Fatal(err) + } + justified, err = testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, uint32(3*thor.CheckpointInterval), block.Number(justified)) + }, + }, { + "get finalized, not justified, then justified", func(t *testing.T) { + type tJustified = justified + testBFT, err := newTestBft(defaultFC) + if err != nil { + t.Fatal(err) + } - // finalized should be head(concluded) - 2 epoch - finalized, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 5) - if err != nil { - t.Fatal(err) + if err = testBFT.fastForward(3*thor.CheckpointInterval - 1); err != nil { + t.Fatal(err) + } + + assert.Equal(t, uint32(thor.CheckpointInterval), block.Number(testBFT.engine.Finalized())) + + if err = testBFT.fastForwardWithMinority(thor.CheckpointInterval); err != nil { + t.Fatal(err) + } + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, uint32(2*thor.CheckpointInterval), block.Number(justified)) + + if err = testBFT.fastForward(thor.CheckpointInterval); err != nil { + t.Fatal(err) + } + justified, err = testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, uint32(4*thor.CheckpointInterval), block.Number(justified)) + // test cache + assert.Equal(t, justified, testBFT.engine.justified.Load().(tJustified).value) + }, + }, { + "fork in the middle, get justified", func(t *testing.T) { + fc := defaultFC + fc.FINALITY = thor.CheckpointInterval + + testBFT, err := newTestBft(fc) + if err != nil { + t.Fatal(err) + } + + for i := 0; i < 2*thor.CheckpointInterval-2; i++ { + if err = testBFT.fastForward(1); err != nil { + t.Fatal(err) + } + + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), justified) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), testBFT.engine.Finalized()) + } + + if err = testBFT.fastForward(1); err != nil { + t.Fatal(err) + } + justified, err := testBFT.engine.Justified() + assert.Nil(t, err) + assert.Equal(t, uint32(thor.CheckpointInterval), block.Number(justified)) + assert.Equal(t, testBFT.repo.GenesisBlock().Header().ID(), testBFT.engine.Finalized()) + }, + }, } - assert.Equal(t, finalized, testBFT.engine.Finalized()) - // justified should be head(concluded) -1 epoch - jc, err = testBFT.repo.NewBestChain().GetBlockID(thor.CheckpointInterval * 6) - if err != nil { - t.Fatal(err) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.testFunc(t) + }) } - actual, err = testBFT.engine.Justified() - assert.Nil(t, err) - assert.Equal(t, jc, actual) } From daf67cc021b6737f15f41a8b70c3e734bb7cb7c5 Mon Sep 17 00:00:00 2001 From: tony Date: Wed, 21 Aug 2024 16:34:10 +0800 Subject: [PATCH 12/12] add comments --- bft/engine.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bft/engine.go b/bft/engine.go index 60762523d..925e469b2 100644 --- a/bft/engine.go +++ b/bft/engine.go @@ -118,6 +118,8 @@ func (engine *BFTEngine) Justified() (thor.Bytes32, error) { return thor.Bytes32{}, err } + // if the quality is 0, then the epoch is not justified + // this is possible for the starting epochs if quality == 0 { return finalized, nil }