Martin/5092 mutator seals #179

arrivets · 2020-11-25T13:47:33Z

This PR addresses issue 5092

Update Mutator sealExtend

Check that seals correspond to IncorporatedResults on the current fork
Create IsValidSeal stub which should check the cryptographic integrity of the
seal and verify that it has collected enough approval signatures based on
the chunk assignment derived from the IncorporatedResult

* Check that seals correspond to IncorporatedResults on the current fork * Create IsValidSeal stub which should check the cryptographic integrity of the seal and verify that it has collected enough approval signatures based on the chunk assignment derived from the IncorporatedResult

…eals

AlexHentschel

This is a great PR; thanks Martin.
Very well documented and tested 👏 .

I only have a couple minor stylistic suggestions. It also would be great to add a few more test cases:

see my suggestion for extending TestExtendSealNoIncorporatedResult
Mutator rejects duplicated seals:
- seal was already included in a parent block
- two seals for the same block are included in the same payload

(In case there are already tests for some of the listed scenarios, my apologies. Feel free to just skip)

state/protocol/badger/mutator.go

state/protocol/badger/mutator_test.go

jordanschalm

Looks good, nice work! 💯

jordanschalm · 2020-11-27T05:09:43Z

state/protocol/badger/mutator.go

+
+		// Collect execution results from receipts. We are visiting the blocks
+		// in ascending order, so if the lookup table already contains an entry
+		// for an execution result, we don't need top updated it.


Suggested change

// for an execution result, we don't need top updated it.

// for an execution result, we don't need to update it.

jordanschalm · 2020-11-27T05:48:49Z

state/protocol/badger/mutator_test.go

@@ -391,24 +391,29 @@ func TestExtendSealedBoundary(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, seal.FinalState, finalCommit, "original commit should be root commit")

+		// Create a first block on top of root
 		first := unittest.BlockWithParentFixture(root.Header)


Since we have both receipts and blocks here with names first..., I'd suggest including the type in the names for both (eg. firstBlock or block1 and receipt1)

jordanschalm · 2020-11-27T05:50:13Z

state/protocol/badger/mutator_test.go

 		require.NoError(t, err)

-		err = state.Mutate().Extend(&second)
+		// Add a third block containg a seal for the first block
+		firstSeal := unittest.SealFixture(unittest.SealFromResult(&firstReceipt.ExecutionResult))


We have variables seal and firstSeal - I'd suggest renaming to seal to rootSeal to disambiguate, especially for the assertions at the end of the test

zhangchiqing · 2020-11-30T18:19:35Z

state/protocol/badger/mutator.go

-
-	// get the parent's block seal, which constitutes the beginning of the
+	// Get the latest seal in the fork that ends with block `ParentID`.
+	// This last sealed block in the fork constitutes the beginning of the
 	// sealing chain; if no seals are part of the payload, it will also be used
 	// for the candidate block, which remains at the same sealed state
 	last, err := m.state.seals.ByBlockID(header.ParentID)
 	if err != nil {


This assumes the parent seal must be found. Could we add a comment explaining why?

Do you mean, why it would be an unexpected internal error if we didn't find the latest seal in the fork?
I think the explanation would be

// Get the latest seal in the fork that ends with block `ParentID`. The protocol state saves this // information for each block that has been successfully added the chain tree. Prior to entering this // method, we check in `headerExtend` that the candidate block's header is a valid extension of // the chain. Reaching this payload check here implies that `headerExtend` passed successfully, // i.e. the parent must already be part of the chain tree. Therefore, _not_ finding the latest sealed block // in the fork up to the parent constitutes a fatal internal error.

Does that sound like a convincing argument?

for each block that has been successfully added the chain tree

worth noting that this is true even for blocks that don't themselves contain any seals

I think the explanation would be

Yeah, thanks for the explanation. I wonder if makes sense to make such dependency more explicit by taking the seal from the input:

lastSealedUpToParentBlock, err := headerExtend(block) err := sealExtend(block, lastSealedUpToParentBlock)

This way, headerExtend must be called before sealExtend, otherwise there is no lastSealedUpToParentBlock.

I am a big proponent of making conceptual dependencies explicit in the code. Thanks for the suggestion Leo.
+1

AlexHentschel

Thanks for cleaning up sealExtend; the code is much more readable now.
Great work!

jordanschalm · 2020-11-30T20:42:39Z

state/protocol/badger/mutator.go

-
-	// get the parent's block seal, which constitutes the beginning of the
+	// Get the latest seal in the fork that ends with block `ParentID`.
+	// This last sealed block in the fork constitutes the beginning of the
 	// sealing chain; if no seals are part of the payload, it will also be used
 	// for the candidate block, which remains at the same sealed state
 	last, err := m.state.seals.ByBlockID(header.ParentID)
 	if err != nil {


for each block that has been successfully added the chain tree

worth noting that this is true even for blocks that don't themselves contain any seals

jordanschalm · 2020-11-30T21:01:00Z

state/protocol/badger/mutator.go

+// sealExtend checks the compliance of the payload seals and returns the last
+// valid seal on the fork. Payload seals should form a valid chain on top of the
+// last seal, and should only correspond to blocks and execution results
+// incorporated on the current fork.


Suggested change

// sealExtend checks the compliance of the payload seals and returns the last

// valid seal on the fork. Payload seals should form a valid chain on top of the

// last seal, and should only correspond to blocks and execution results

// incorporated on the current fork.

// sealExtend checks the compliance of the payload seals and returns the last

// valid seal on the fork up to and including `candidate`. Payload seals should form a valid chain on top of the

// last seal as of the parent of `candidate`, and should only correspond to blocks and execution results

// incorporated on the current fork.

state/protocol/badger/mutator.go

utils/unittest/epoch_builder.go

* add lastSealUpToParent parameter to make the dependency on checking the existance of this seal explicit in the code * add comment about implicit validation of the sub-graph check

…eals

AlexHentschel

Thanks. The Mutator is very clean now :-)
From my perspective, the PR is ready to be merged without any further modifications.

AlexHentschel · 2020-12-03T02:01:28Z

state/protocol/badger/mutator.go

+func NewMutator(state *State) *Mutator {
+	return &Mutator{
+		state: state,
+	}
+}
+


Do we still need the constructor? We now have the

flow-go/state/protocol/badger/state.go

Lines 21 to 25 in 4dd3d6e

// MutatorFactory is an interface that is used to build Mutator instance

// it is used to inject different implementations for badger.State.

type MutatorFactory interface {

Create(state *State) protocol.Mutator

}

Suggested change

func NewMutator(state *State) *Mutator {

return &Mutator{

state: state,

}

}

I think the implementation is also not setting the required filed validator

flow-go/state/protocol/badger/mutator.go

Lines 20 to 23 in 4dd3d6e

type Mutator struct {

state *State

validator module.ReceiptValidator

}

The diff contains: - a lot of changes that concern parts of the library we don't use (other curves a la BN, BLS24-X, BLS12-383 ...), integer protocols (ETRS), field extension machinery ... - otherwise irrelevant changes, e.g. CI/CD - some memory bug fixing [Full Changeset](https://github.com/relic-toolkit/relic/compare/7a9bba7f..9206ae5) **Fixed bugs:** - Unexpected failure of ep2\_mul\[\_lwnaf\] above the prime group order [\onflow#64](relic-toolkit/relic#64) **Closed issues:** - Other way to construct towered extension fields [\onflow#203](relic-toolkit/relic#203) - blake2.h:101:5: error: size of array element is not a multiple of its alignment [\onflow#202](relic-toolkit/relic#202) - ECIES 160bit [\onflow#201](relic-toolkit/relic#201) - Compilation with "ARITH gmp" fails [\onflow#200](relic-toolkit/relic#200) - Support for armv8-a ? [\onflow#198](relic-toolkit/relic#198) - Function name bn\_init conflicts with OpenSSL when used in tandem [\onflow#196](relic-toolkit/relic#196) - 16-bit MSP430 [\onflow#193](relic-toolkit/relic#193) - Modular exponentiation returns 1 if exponent is 0 and modulo is 1 [\onflow#185](relic-toolkit/relic#185) - Compilation of RELIC with bls12-446 and bls12-455 fails [\onflow#182](relic-toolkit/relic#182) - test\_bn fails with BLS12-381 preset [\onflow#181](relic-toolkit/relic#181) - \[BUG\] undefined reference to `bench_init', `bench\_clean' [\onflow#180](relic-toolkit/relic#180) - Tests FTBFS because of missing symbol in header [\onflow#179](relic-toolkit/relic#179) - Builds are broken [\onflow#178](relic-toolkit/relic#178) - compile error inlining failed in call to always\_inline ‘\_mm\_alignr\_epi8’ on unbantu20.04 gcc9 [\onflow#177](relic-toolkit/relic#177) - bn\_write\_str buffer overflow [\onflow#176](relic-toolkit/relic#176) - ECDSA verify succeeds when it should fail [\onflow#175](relic-toolkit/relic#175) - ec\_mul\_gen hangs with curve SECG\_K256 [\onflow#174](relic-toolkit/relic#174) - Wrong square root computation [\onflow#173](relic-toolkit/relic#173) - Out-of-bounds read via bn\_sqr\_basic [\onflow#172](relic-toolkit/relic#172) - OSS-Fuzz integration [\onflow#171](relic-toolkit/relic#171) - Building Relic with Curve NIST\_P256 throws FATAL ERROR in relic\_fp\_prime.c:120 [\onflow#170](relic-toolkit/relic#170) - Compressing \(packing\) a point to binary array does not comply with X9.62 standard [\onflow#169](relic-toolkit/relic#169) - ‘ctx\_t’ {aka ‘struct \_ctx\_t’} has no member named ‘total’ [\onflow#168](relic-toolkit/relic#168) - relic does not work with C++ [\onflow#167](relic-toolkit/relic#167) - Memory leak in ep2\_curve\_init/clean with ALLOC=DYNAMIC [\onflow#166](relic-toolkit/relic#166) - \*\_is\_valid\(\) functions produce false negative for not normalized points [\onflow#147](relic-toolkit/relic#147) - Bench and Test doesnt build [\onflow#122](relic-toolkit/relic#122) **Merged pull requests:** - Add pairing delegation protocols [\onflow#199](relic-toolkit/relic#199) ([dfaranha](https://github.com/dfaranha)) - Fix support for Win64/MSVC targets. [\onflow#197](relic-toolkit/relic#197) ([dfaranha](https://github.com/dfaranha)) - Simplify generator getting for Gt. [\onflow#194](relic-toolkit/relic#194) ([luozejiaqun](https://github.com/luozejiaqun)) - cmake: Always use user defined CFLAGS, not only for release builds [\onflow#187](relic-toolkit/relic#187) ([xdustinface](https://github.com/xdustinface)) - Fix MinGW build [\onflow#186](relic-toolkit/relic#186) ([xdustinface](https://github.com/xdustinface)) - Remove debug printf in bn\_mxp\_slide [\onflow#184](relic-toolkit/relic#184) ([guidovranken](https://github.com/guidovranken)) - Remove ALLOC = STACK to simplify memory allocation. [\onflow#183](relic-toolkit/relic#183) ([dfaranha](https://github.com/dfaranha)) - Update relic\_alloc.h [\onflow#165](relic-toolkit/relic#165) ([aguycalled](https://github.com/aguycalled)) - Add correct support for FreeBSD and NetBSD [\onflow#164](relic-toolkit/relic#164) ([hoffmang9](https://github.com/hoffmang9))

arrivets requested review from AlexHentschel, yhassanzadeh13 and zhangchiqing as code owners November 25, 2020 13:47

arrivets changed the base branch from feature/new-sealing-phase-2 to martin/chunk-based-sealing November 25, 2020 13:56

arrivets requested review from jordanschalm, m4ksio, psiemens, ramtinms and tarakby as code owners November 25, 2020 13:56

arrivets changed the base branch from martin/chunk-based-sealing to feature/new-sealing-phase-2 November 25, 2020 13:56

arrivets changed the base branch from feature/new-sealing-phase-2 to master November 25, 2020 14:28

arrivets changed the base branch from master to feature/new-sealing-phase-2 November 25, 2020 14:29

Merge branch 'feature/new-sealing-phase-2' into martin/5092-mutator-s…

e85a64f

…eals

AlexHentschel approved these changes Nov 27, 2020

View reviewed changes

jordanschalm approved these changes Nov 27, 2020

View reviewed changes

arrivets added 2 commits November 27, 2020 17:07

Refactor Mutator sealExtend and add tests

42fe9da

Update EpochBuilder with sealing phase 2 requirements

551e500

zhangchiqing reviewed Nov 30, 2020

View reviewed changes

AlexHentschel approved these changes Nov 30, 2020

View reviewed changes

jordanschalm reviewed Nov 30, 2020

View reviewed changes

arrivets added 4 commits December 1, 2020 10:09

Update sealExtend comments

e0b0eee

Update sealExtend

54d574e

* add lastSealUpToParent parameter to make the dependency on checking the existance of this seal explicit in the code * add comment about implicit validation of the sub-graph check

Merge branch 'feature/new-sealing-phase-2' into martin/5092-mutator-s…

beb0d66

…eals

Fix tests with mock ReceiptValidator

4dd3d6e

AlexHentschel approved these changes Dec 3, 2020

View reviewed changes

Remove NewMutator constructor

4762d56

arrivets merged commit e92bbbd into feature/new-sealing-phase-2 Dec 3, 2020

arrivets deleted the martin/5092-mutator-seals branch December 3, 2020 10:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Martin/5092 mutator seals #179

Martin/5092 mutator seals #179

arrivets commented Nov 25, 2020 •

edited

Loading

AlexHentschel left a comment •

edited

Loading

jordanschalm left a comment

jordanschalm Nov 27, 2020

jordanschalm Nov 27, 2020

jordanschalm Nov 27, 2020

zhangchiqing Nov 30, 2020

AlexHentschel Nov 30, 2020

jordanschalm Nov 30, 2020

zhangchiqing Dec 1, 2020 •

edited

Loading

AlexHentschel Dec 1, 2020

AlexHentschel left a comment •

edited

Loading

jordanschalm Nov 30, 2020

jordanschalm Nov 30, 2020

AlexHentschel left a comment

AlexHentschel Dec 3, 2020

	// for an execution result, we don't need top updated it.
	// for an execution result, we don't need to update it.

	// MutatorFactory is an interface that is used to build Mutator instance
	// it is used to inject different implementations for badger.State.
	type MutatorFactory interface {
	Create(state *State) protocol.Mutator
	}

	type Mutator struct {
	state *State
	validator module.ReceiptValidator
	}

Martin/5092 mutator seals #179

Martin/5092 mutator seals #179

Conversation

arrivets commented Nov 25, 2020 • edited Loading

AlexHentschel left a comment • edited Loading

Choose a reason for hiding this comment

jordanschalm 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

zhangchiqing Dec 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexHentschel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arrivets commented Nov 25, 2020 •

edited

Loading

AlexHentschel left a comment •

edited

Loading

zhangchiqing Dec 1, 2020 •

edited

Loading

AlexHentschel left a comment •

edited

Loading