feat: chainstore: FRC-0051: Remove all equivocated blocks from tipsets #11104

arajasek · 2023-07-27T14:36:14Z

Related Issues

Proposed Changes

As discussed in #10793, we now reject ALL blocks with the same miner and same height (irrespective of other details like common parents, common ticket, etc.). Further, when asked to consider taking a new tipset as head, we first refresh our heaviest tipset in order to check whether it's weight has DECREASED as a result of equivocation.

This PR also refactors / simplifies much of the "taking heavier tipset" logic. We now simply:

PersistTipsets as we receive them, which also writes the tipset keys to the blockstore
AddToTipsetTracker as we validate tipsets (blocks)
When a new "head" is synced to / new block comes in, we simply notify the chainstore of its existence by calling RefreshHeaviestTipSet with the height of the new tipset.
RefreshHeaviestTipSet works by:
- Forming the best tipset at its CURRENT head's height -- if this best tipset is LIGHTER, we immediately take it as our new head, since that implies equivocation
- Forming the best tipset at the input height (assuming different from height in previous step)
- taking the heaviest of these tipsets as new head

Additional Info

Checklist

Before you mark the PR ready for review, please make sure that:

Commits have a clear commit message.
PR title is in the form of of <PR type>: <area>: <change being made>
- example: fix: mempool: Introduce a cache for valid signatures
- PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
- area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
New features have usage guidelines and / or documentation updates in
- Lotus Documentation
- Discussion Tutorials
Tests exist for new functionality or change in behavior
CI is green

arajasek · 2023-07-27T14:36:28Z

chain/store/store.go

@@ -1027,7 +1027,7 @@ func (cs *ChainStore) persistBlockHeaders(ctx context.Context, b ...*types.Block
 	return err
 }

-func (cs *ChainStore) expandTipset(ctx context.Context, b *types.BlockHeader) (*types.TipSet, error) {
+func (cs *ChainStore) ExpandTipset(ctx context.Context, b *types.BlockHeader) (*types.TipSet, error) {


Exported for testing.

arajasek · 2023-07-27T14:37:39Z

chain/store/store.go

@@ -1052,7 +1052,15 @@ func (cs *ChainStore) expandTipset(ctx context.Context, b *types.BlockHeader) (*
 		}

 		if cid, found := inclMiners[h.Miner]; found {
-			log.Warnf("Have multiple blocks from miner %s at height %d in our tipset cache %s-%s", h.Miner, h.Height, h.Cid(), cid)
+			log.Warnf("Have multiple blocks from miner %s at height %d in our tipset cache %s-%s, purging them all", h.Miner, h.Height, h.Cid(), cid)
+			newAll := make([]*types.BlockHeader, 0, len(all))


We could ALSO drop the blocks from cs.tipsets entirely if we wanted to. I'm not sure this is the best thing to do, though, since that tracker is currently split between the "writer" method AddToTipsetTracker and the "reader" method ExpandTipset.

I'd leave them so we can detect new ones.

Stebalien · 2023-07-27T14:45:16Z

chain/store/store.go

@@ -1052,7 +1052,15 @@ func (cs *ChainStore) expandTipset(ctx context.Context, b *types.BlockHeader) (*
 		}

 		if cid, found := inclMiners[h.Miner]; found {
-			log.Warnf("Have multiple blocks from miner %s at height %d in our tipset cache %s-%s", h.Miner, h.Height, h.Cid(), cid)
+			log.Warnf("Have multiple blocks from miner %s at height %d in our tipset cache %s-%s, purging them all", h.Miner, h.Height, h.Cid(), cid)
+			newAll := make([]*types.BlockHeader, 0, len(all))


I'd leave them so we can detect new ones.

Stebalien · 2023-07-27T14:46:39Z

chain/store/store.go

+			newAll := make([]*types.BlockHeader, 0, len(all))
+			for _, blk := range all {
+				if blk.Miner != h.Miner {
+					newAll = append(newAll, blk)


nit: I'd just collect all these blocks into a miner -> block map, then put them into an array at the very end.

arajasek · 2023-07-31T14:34:01Z

chain/gen/gen.go

+		return nil, xerrors.Errorf("failed to persist tipset: %w", err)
+	}
+
+	for _, blk := range fts.TipSet().Blocks() {


Not strictly necessary since we do this in PutTipSet too, but doesn't seem harmful to do for tests.

guy-goren

I left comments (mostly nitpicks) and questions. Be aware that I'm neither a proficient coder nor am I familiar with this code base (hence, possibly trivial questions).
My two main comments are:

I think that conceptually it's better to consider a winning ticket rather than the proposing miner. Is there an implementation reason for considering the miner?
If we don't keep the equivocating blocks (or just their cids), we may run into future trouble when we'd like to prove the miner's bad behavior (i.e., create an irrefutable proof of misbehavior).

guy-goren · 2023-08-01T12:51:13Z

chain/store/store.go

+		if blkCid, found := inclMiners[h.Miner]; found {
+			log.Warnf("Have multiple blocks from miner %s at height %d in our tipset cache %s-%s, purging them all", h.Miner, h.Height, h.Cid(), blkCid)
+			badMiners[h.Miner] = struct{}{}
+			delete(inclMiners, h.Miner)


If you delete h.Miner from the inclMiners, then wouldn't the next block from the equivocating miner (e.g., the 3rd block with the same winning ticket) gets included?
If this is not the case (because badMiners is used for that), then what's the benefit of badMiners instead of just looking in inclMiners again? (Seems inefficient.)

guy-goren · 2023-08-01T12:55:05Z

chain/store/store.go

-		if types.CidArrsEqual(h.Parents, b.Parents) {
-			all = append(all, h)
-			inclMiners[h.Miner] = bhc
+		if blkCid, found := inclMiners[h.Miner]; found {


I think it's better to reject based on the same ticket rather than the same miner.

guy-goren · 2023-08-01T13:30:48Z

chain/store/store.go

-	all := []*types.BlockHeader{b}
-
-	tsets, ok := cs.tipsets[b.Height]
+	blocks, ok := cs.tipsets[height]


Are these blocks from a SINGLE tipset at that height or can these contain blocks from different tipsets with the same height (possible forks)?

guy-goren · 2023-08-01T13:41:52Z

chain/store/store.go

@@ -1027,44 +1043,51 @@ func (cs *ChainStore) persistBlockHeaders(ctx context.Context, b ...*types.Block
 	return err
 }

-func (cs *ChainStore) expandTipset(ctx context.Context, b *types.BlockHeader) (*types.TipSet, error) {
+func (cs *ChainStore) ExpandTipsetByHeightAndParent(ctx context.Context, height abi.ChainEpoch, parentTipsets []cid.Cid) (types.TipSetKey, error) {


Is parntTipsets a single tipset or could there be multiple parent tipsets?

guy-goren · 2023-08-01T13:50:24Z

chain/store/store.go

+	}
+
+	var ret []cid.Cid
+	for _, blk := range inclMiners {


Extending on a previous comment.
Personally, I find it confusing the use of included miners instead of included blocks (winning tickets). Currently there's a 1-1 relation between the two, but who's to say that this relation will remain in the future? Maybe there would be an implementation where a "very lucky" miner gets to include more blocks in a single tipset?
There's probably nothing you can do with it here, but maybe you have a reason that I failed to understand.

arajasek · 2023-08-02T19:48:52Z

chain/store/store.go

 	if err := cs.PersistTipsets(ctx, []*types.TipSet{ts}); err != nil {
 		return xerrors.Errorf("failed to persist tipset: %w", err)
 	}

-	expanded, err := cs.expandTipset(ctx, ts.Blocks()[0])
+	expanded, err := cs.ExpandTipsetByHeightAndParent(ctx, ts.Height(), ts.Parents().Cids())


I'm tempted to just use the new FormBestTipSetForHeight method here as well, and drop ExpandTipsetByHeightAndParent entirely.

Any reason not to?

It's a bit slower, but I think I'd prefer doing it anyway

Stebalien

Some initial comments but I want to walk through this together tomorrow.

Stebalien · 2023-08-03T22:49:31Z

chain/store/store.go

+			return xerrors.Errorf("failed to add block to ts tracker: %w", err)
+		}
+	}
+
 	if err := cs.PersistTipsets(ctx, []*types.TipSet{ts}); err != nil {


Can we persist inside AddToTipSetTracker? Or do it first?

I think the right sequence of steps is:

Persist, when you receive a block of interest

Track, when you actually validate it

Assume in "PutTipSet" that Persisting and Tracking has already happened

I'm gonna implement that and state the assumption for now.

Stebalien · 2023-08-03T22:49:59Z

chain/store/store.go

 	if err := cs.PersistTipsets(ctx, []*types.TipSet{ts}); err != nil {
 		return xerrors.Errorf("failed to persist tipset: %w", err)
 	}

-	expanded, err := cs.expandTipset(ctx, ts.Blocks()[0])
+	expanded, err := cs.ExpandTipsetByHeightAndParent(ctx, ts.Height(), ts.Parents().Cids())


Any reason not to?

Stebalien · 2023-08-03T23:09:57Z

chain/store/store.go

+	return types.NewTipSetKey(ret...), nil
+}
+
+func (cs *ChainStore) FormBestTipSetForHeight(ctx context.Context, height abi.ChainEpoch) (*types.TipSet, types.BigInt, error) {


This should not be exported.

Only exported for tests, which need to be in a separate package (since they depend on the chain generator, which depends on the chainstore...)

Ugh.... ok.

Stebalien · 2023-08-03T23:10:27Z

chain/store/store.go

+	if cs.heaviest != nil {
+		heaviestHeight = cs.heaviest.Height()
+	}
+	newHeaviest, newHeaviestWeight, err := cs.FormBestTipSetForHeight(ctx, heaviestHeight)


nit: FormHeaviestTipSetForHeight?

Stebalien · 2023-08-03T23:10:54Z

chain/store/store.go

@@ -409,7 +420,7 @@ func (cs *ChainStore) PutTipSet(ctx context.Context, ts *types.TipSet) error {
 // MaybeTakeHeavierTipSet evaluates the incoming tipset and locks it in our
 // internal state as our new head, if and only if it is heavier than the current
 // head and does not exceed the maximum fork length.
-func (cs *ChainStore) MaybeTakeHeavierTipSet(ctx context.Context, ts *types.TipSet) error {
+func (cs *ChainStore) MaybeTakeHeavierTipSet(ctx context.Context, newTsk types.TipSetKey) error {


can we unexport this method?

and may be call it informNewTipSet?

Basically, make it clear that we're telling the system "hey, we have this new tipset", maybe take it, maybe take something entirely different!

Stebalien · 2023-08-03T23:11:54Z

chain/store/store.go

+
+	if newHeaviest == nil || newHeaviestWeight.LessThan(heaviestWeight) {
+		log.Warnf("chainstore heaviest tipset's weight SHRANK from %d (%s) to %d (%s) due to equivocation", heaviestWeight, cs.heaviest, newHeaviestWeight, newHeaviest)
+		// refresh heaviestWeight 10 times moving up and down


Needs lots of comments.

Still needs some comments explaining why.

chain/store/store.go

Stebalien

nits, but this looks correct

Stebalien · 2023-08-09T21:02:59Z

chain/sync.go

@@ -247,6 +247,7 @@ func (syncer *Syncer) InformNewHead(from peer.ID, fts *store.FullTipSet) bool {
 		return false
 	}

+	// TODO: this method name is a lie


Can we document why it's a lie?

Can we add a 🍰 emoji?

Stebalien · 2023-08-09T21:11:50Z

chain/store/store.go

-	expanded, err := cs.expandTipset(ctx, ts.Blocks()[0])
-	if err != nil {
-		return xerrors.Errorf("errored while expanding tipset: %w", err)
+	if err := cs.AddToTipSetTracker(ctx, b); err != nil {


nit: it would be nice if this just handled writing blocks internally, but I guess that's probably not critical right now?

Yeah, not critical I think. Also not super easy to do, because we want to be batching those writes, and this method tracks things one block at a time.

Stebalien · 2023-08-09T21:13:14Z

chain/store/store.go

-		if err = cs.chainLocalBlockstore.Put(ctx, tsBlk); err != nil {
-			return xerrors.Errorf("failed to put tipset key block: %w", err)
-		}
+	if err := cs.RefreshHeaviestTipSet(ctx, ts.Height()); err != nil {


Hm... for genesis, are we sure any of this is right? Shouldn't we force replace everything? Like:

Discard all tipsets.

Replace the first tipset.

Move on?

Otherwise, e.g., the metadataDs.Put line below could do the wrong thing.

(not like this is likely to cause issues, but it's still a bit funky)

I think you're right, but I was going for matching existing behaviour here, which this should do?

Yeah, you're right.

Let's just keep a list of TODOs, merge this, then we can make followup PRs (if/when we get time).

Stebalien · 2023-08-09T21:16:25Z

chain/store/store.go

+
+	if newHeaviest == nil || newHeaviestWeight.LessThan(heaviestWeight) {
+		log.Warnf("chainstore heaviest tipset's weight SHRANK from %d (%s) to %d (%s) due to equivocation", heaviestWeight, cs.heaviest, newHeaviestWeight, newHeaviest)
+		// refresh heaviestWeight 10 times moving up and down


Still needs some comments explaining why.

Stebalien · 2023-08-09T21:18:11Z

chain/store/store.go

+	}
+
+	// if the new height we were notified about isn't what we just refreshed at, see if we have a heavier tipset there
+	if newTsHeight != newHeaviest.Height() {


I'm not sure how this is possible? If we refresh at a different height, shouldn't we have picked the heaviest tipset at that height?

No, not necessarily. We'll only refresh at different heights if there was equivocation. Basically, assume current height heaviestHeight is 10, and we were informed about newTsHeight of 11.

We refresh by finding the heaviest tipset at 10

If the "refreshed" tipset at 10 is not lighter than our current heaviest tipset at 10, we'll just move on

At that point, we hit this line, and decide we should form the heaviest tipset at height 11, and compare the two.

(If the "refreshed" tipset WAS lighter, though, then yes we would likely form a heavier tipset at 11, and immediately take that as our head, in which case we'd short-circuit here)

I think I understand?

Let's talk through tomorrow, but I'm pretty sure it's correct.

Stebalien · 2023-08-09T21:24:17Z

chain/store/store.go

-	// fix pretty easily
+// FormHeaviestTipSetForHeight looks up all valid blocks at a given height, and returns the heaviest tipset that can be made at that height
+// It does not consider ANY blocks from miners that have "equivocated" (produced 2 blocks at the same height)
+func (cs *ChainStore) FormHeaviestTipSetForHeight(ctx context.Context, height abi.ChainEpoch) (*types.TipSet, types.BigInt, error) {


This looks correct and probably "fast enough". But I'd like to poke at making it faster in the future by pre-grouping by height, etc.

Stebalien · 2023-08-09T22:09:41Z

chain/store/store.go

+	}
+
+	// if the new height we were notified about isn't what we just refreshed at, see if we have a heavier tipset there
+	if newTsHeight != newHeaviest.Height() {


I think I understand?

Stebalien · 2023-08-09T22:18:42Z

Ok, actually. I'm sorry, but I'm going to need to do this: is there any way to write a simple equivocation test?

arajasek · 2023-08-09T22:29:40Z

Ok, actually. I'm sorry, but I'm going to need to do this: is there any way to write a simple equivocation test?

That's at least kiiinda what this test is intended to be. Definitely others that could be written, though...

Stebalien · 2023-08-09T23:27:22Z

(but it would be nice to get one that tests switching away from a tipset)

arajasek · 2023-08-09T23:41:53Z

@Stebalien Added more in latest commit, please suggest more if you think of them -- we need to get this right.

Stebalien · 2023-08-10T00:03:55Z

Well, the tests I'd like are:

We switch tipsets at the current height (needs multiple tipsets with different parents).
We switch to a different height because we lose all the blocks in the tipset.
And maybe some kind of validation that we don't crash with too many null blocks in case 2?

arajasek · 2023-08-10T01:02:57Z

@Stebalien Thanks, all good ideas. I've added all 3 cases in latest commit, please take a look.

Stebalien

LGTM!

arajasek requested a review from a team as a code owner July 27, 2023 14:36

arajasek commented Jul 27, 2023

View reviewed changes

arajasek requested a review from Stebalien July 27, 2023 14:38

arajasek mentioned this pull request Jul 27, 2023

FRC0051 Implementation Proposal #10793

Closed

Stebalien approved these changes Jul 27, 2023

View reviewed changes

arajasek force-pushed the asr/frc-0051 branch 3 times, most recently from 5ac13fb to 0904d80 Compare July 31, 2023 14:32

arajasek commented Jul 31, 2023

View reviewed changes

arajasek force-pushed the asr/frc-0051 branch from 0904d80 to 0bceece Compare July 31, 2023 15:28

guy-goren reviewed Aug 1, 2023

View reviewed changes

jennijuju requested a review from Kubuxu August 1, 2023 18:22

jennijuju linked an issue Aug 1, 2023 that may be closed by this pull request

FRC0051 Implementation Proposal #10793

Closed

arajasek removed a link to an issue Aug 1, 2023

FRC0051 Implementation Proposal #10793

Closed

arajasek force-pushed the asr/frc-0051 branch from 0bceece to 85525d0 Compare August 2, 2023 19:47

arajasek commented Aug 2, 2023

View reviewed changes

arajasek force-pushed the asr/frc-0051 branch 4 times, most recently from 53c1520 to b6504dc Compare August 3, 2023 20:47

Stebalien reviewed Aug 3, 2023

View reviewed changes

arajasek force-pushed the asr/frc-0051 branch 4 times, most recently from c7c4bdc to 261a398 Compare August 9, 2023 21:23

Stebalien approved these changes Aug 9, 2023

View reviewed changes

arajasek added 3 commits August 9, 2023 19:28

feat: chainstore: FRC-0051: Remove all equivocated blocks from tipsets

52657c5

Address review

0ff3349

add more test cases covering chainstore equivocation

2cbdedc

arajasek force-pushed the asr/frc-0051 branch from 7e80842 to 2cbdedc Compare August 9, 2023 23:40

add more test cases covering chainstore equivocation

69f3580

Stebalien approved these changes Aug 10, 2023

View reviewed changes

arajasek merged commit 49010a9 into master Aug 10, 2023

arajasek deleted the asr/frc-0051 branch August 10, 2023 01:50

This was referenced Aug 10, 2023

feat: miner: implement FRC-0051 #11157

Merged

fix: chainstore: do not get stuck in unhappy equivocation cases #11159

Merged

feat: chainstore: FRC-0051: Remove all equivocated blocks from tipsets #11104

feat: chainstore: FRC-0051: Remove all equivocated blocks from tipsets #11104

Conversation

arajasek commented Jul 27, 2023 • edited Loading

Related Issues

Proposed Changes

Additional Info

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guy-goren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien commented Aug 9, 2023

arajasek commented Aug 9, 2023

Stebalien commented Aug 9, 2023

arajasek commented Aug 9, 2023

Stebalien commented Aug 10, 2023

arajasek commented Aug 10, 2023

Stebalien left a comment

Choose a reason for hiding this comment

arajasek commented Jul 27, 2023 •

edited

Loading