fix: some fixes and refactoring for stabilizing same-block execution by shotonoff · Pull Request #451 · dashpay/tenderdash

shotonoff · 2022-09-01T12:18:25Z

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have added or updated relevant unit/integration/functional/e2e tests
I have made corresponding changes to the documentation

For repository code-owners and collaborators only

I have assigned this pull request to a milestone

…execution-stabilizing

…re_v2.go

…execution-stabilizing

abci/tests/server/client.go

cmd/tenderdash/commands/reindex_event.go

internal/state/state.go

internal/state/current_round_state.go

lklimek · 2022-09-02T09:05:30Z

internal/state/current_round_state.go


-	target.NextValidatorsHash = candidate.NextValidators.Hash()
+	if candidate.NextValidators != nil {
+		target.NextValidatorsHash = candidate.NextValidators.Hash()


The idea here was that all fields of Candidate contain final values and already apply reasonable defaults.

It means this if should be handled somehow in populateValsetUpdates.

Maybe the issue is that populateValsetUpdates() was not called at all? Otherwise NextValidators should be derived from current state.

internal/consensus/helper.go

internal/consensus/common_test.go

internal/consensus/reactor_test.go

lklimek · 2022-09-05T11:58:34Z

internal/consensus/reactor_test.go

+	count int,
+) (*quorumData, error) {
+	switch operation {
+	case "add":


use enum or const

…node has proposal block for the height

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

…' into same-block-execution-stabilizing

…execution-stabilizing

shotonoff

feedback work done

shotonoff · 2022-09-06T09:53:44Z

abci/example/kvstore/kvstore_test.go

-
-	kvVals, err = kvstore.ValidatorSet()
+	require.Equal(t, initVals.QuorumHash, resp.ValidatorSetUpdate.QuorumHash)
+	_, err = kvstore.FinalizeBlock(ctx, &types.RequestFinalizeBlock{Height: 1, AppHash: resp.AppHash})


I didn't use makeApplyBlock because of I need a process proposal response to check a quorum-hash.
I a bit refactored makeApplyBlock to return responses of process-proposal and finalize-block

cmd/tenderdash/commands/reindex_event.go

internal/consensus/helper.go

shotonoff · 2022-09-06T10:21:39Z

internal/consensus/replay_test.go

+		cfg:       cfg,
+		nPeers:    nPeers,
+		nVals:     nVals,
+		testName:  "replay_test",


I removed testName field, instead I use t.Name() inside generate method

shotonoff · 2022-09-07T08:28:13Z

abci/example/kvstore/kvstore.go

+		logger:              logger.With("module", "kvstore"),
+		lastCommittedState:  loadState(db),
+		roundStates:         map[string]State{},
+		validatorSetUpdates: make(map[int64]types.ValidatorSetUpdate),


as I see storing validator-set-updates in memory is enough to pass tests (so far), let's postpone it until it will be necessary, wdyt?

internal/state/state.go

shotonoff · 2022-09-07T08:29:29Z

internal/state/current_round_state.go

 	target.LastResultsHash = candidate.ResultsHash

-	target.ConsensusParams = candidate.NextConsensusParams
+	if !candidate.NextConsensusParams.IsZero() {


good thoughts for investigation

abci/tests/server/client.go

lklimek · 2022-09-07T13:54:02Z

abci/types/types.go

-	status := ResponseVerifyVoteExtension_REJECT
-	if ok {
-		status = ResponseVerifyVoteExtension_ACCEPT
+func (m *ValidatorSetUpdate) MarshalJSON() ([]byte, error) {


Maybe for m==nil, we should return json.Marshal(nil) ?

not necessary, if a pointer is nill then a result of marshalling will be nil as well

got "marshaling error: json: error calling MarshalJSON for type *types.ValidatorSetUpdate: fromproto: key is nil"

lklimek · 2022-09-07T13:54:42Z

abci/types/types.go

+	NodeAddress string          `json:"node_address"`
+}
+
+func (m *ValidatorUpdate) MarshalJSON() ([]byte, error) {


Maybe for m==nil, we should return json.Marshal(nil) ?

answered above

lklimek · 2022-09-07T13:58:50Z

internal/consensus/helper_test.go

+func (g *nodeGen) initApp(ctx context.Context, t *testing.T) {
 	if g.app == nil {
-		g.app = kvstore.NewApplication()
+		g.app = kvstore.NewApplication(kvstore.WithLogger(log.NewNopLogger()))


NewNopLogger should be a default value, no need to use WithLogger in this case.

And, I think it should be NewTestLogger()

internal/proxy/client_test.go

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

* refactor: decouple dash parameters from a MakeBlock function to a Block.SetDashParams method * feat: add node's pro-rtx-hash into a context * feat: add proTxHash into context for consensus/reactor_test.go * feat: some modifications for PoC same-block execution * feat: change an updated struct from round state on uncommitted-state * feat: support next-validators as for next-block execution mode * feat: support next-validators for same-block execution mode * feat: some modifications for next-core-lock * chore: same block execution state/state_test.go (WIP) * test(state): fix TestManyValidatorChangesSaveLoad() * fix(state): panic on empty consensus params load * chore(state): remove state.NextValidators * chore(proto/abci): same block execution protobuf changes * refactor(types): centralize generation of ResultsHash * refactor(state): consolidate redundant code in block execution * chore(state) execution fix for new data structures * test(state): adapt TestProposerPriorityProposerAlternates for same block exec * test(state): same block exec TestFourAddFourMinusOneGenesisValidators * test(state): same block exec TestStoreLoadValidatorsIncrementsProposerPriority * test(state): same block exec for TestManyValidatorChangesSaveLoad * fix(state): same block exec TestConsensusParamsChangesSaveLoad * test(state): fix TestProcessProposal * chore(state): correct handling of ApplyBlock etc * test(mempool): builds but fails * refactor(state): UncommittedState now holds final values instead of updates This was needed for clean implementation of NextValidatorsHash * refactor(state): rename UncommittedChanges to Changeset * chore: same block exec - block * chore(state): state ID - start of work * chore: SBE - block fields (WIP) * chore: SBE chainlocks and proposed app version (WIP) * test(state): SBE green (execution|state|validation)_test * chore(abci): improve validation of prepare/process proposal responses * chore(state): apphash defaults to array containing zeros * test(state): TestStoreLoadValidators fixed for SBE * chore(state): AppHash defaults to slice of 0s instead of nil * chore(state): fix invalid state.LastStateID * test(state): SBE TestPruneStates * test(state): fix rollback_test * fix(abci): correct handling of AppHash in Application * test(store): update for SBE * chore(types): rename stateID.LastAppHash to AppHash * refactor(statesync): state provider returns AppHash as HexBytes * test(statesync): fix for SBE * test(consensus): show test logs on console * test(consensus): fix byzantine test * test(blocksync): update for SBE * fix(state): events fired twice * fix(consensus): proposal contains invalid chainlock height * test(consensus): correct chainlock in initial block in SBE * Update abci/example/kvstore/kvstore.go Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * Update abci/example/kvstore/kvstore.go Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * fix(state): double commits during replay * test(consensus): fix replay handshake* tests (WIP) * fix: evidence package * fix: validator_conn_executor_test.go * refactor: executing kvstore * feat!(kvstore): same-block-execution KV store (#447) * feat!(kvstore): same-block-execution KV store * fix(kvstore): dstIter not closed correctly * fix(kvstore): AppHash at genesis should be zero * fix(kvstore): SBE support for persistent KV store (#452) * test(proxy): fix test of Info() * fix: some fixes and refactoring for stabilizing same-block execution (#451) * fix: some fixes and refactoring for stabilizing same-block execution * refactor: merge kvstore.App with kvstore.Application and remove kvstore_v2.go * fix: kvstore_test.go * fix: TestValUpdates * fix: TestValUpdates * fix: TestKVStore * fix: rpc/client tests * fix: make stable some tests affected by kvstore refactoring * fix: update uncommitted state if a validator out of quorum set and a node has proposal block for the height * Update internal/consensus/replay_test.go Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: changes according to PR feedback * fix: blocking wal_test.go because of default context * fix: code style issues * fix: abci-cli test * fix: typo in struct name CurentRoundState => CurrentRoundState * fix: a few data race issues * fix: TestHandshakeReplayNone * fix: ValidatorUpdate.MarshalJSON * fix: ValidatorUpdate.UnmarshalJSON * chore(e2e): prototype e2e test app for same-block execution * fix(consensus): state is nil on genesis * fix(state): invalid stateID height on genesis * chore: improve logging * Update abci/tests/server/client.go Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * Update abci/tests/server/client.go Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * Update internal/proxy/client_test.go Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * refactor: changes according to PR feedback Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com> * fix: stabilize codebase after merge / resolve conflicts Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com>

fix: some fixes and refactoring for stabilizing same-block execution

bcb5399

shotonoff requested review from QuantumExplorer and lklimek as code owners September 1, 2022 12:18

shotonoff added 7 commits September 1, 2022 17:26

Merge branch 'proof-of-concept-same-block-execution' into same-block-…

0f71b94

…execution-stabilizing

refactor: merge kvstore.App with kvstore.Application and remove kvsto…

515ca0b

…re_v2.go

fix: kvstore_test.go

0b94f2c

fix: TestValUpdates

6b4bd69

Merge branch 'proof-of-concept-same-block-execution' into same-block-…

53136c9

…execution-stabilizing

fix: TestValUpdates

dc9096d

fix: TestKVStore

2209e8c

lklimek reviewed Sep 5, 2022

View reviewed changes

shotonoff and others added 12 commits September 5, 2022 15:40

fix: rpc/client tests

3d798ca

fix: make stable some tests affected by kvstore refactoring

51574e6

fix: update uncommitted state if a validator out of quorum set and a …

3ff7917

…node has proposal block for the height

Update internal/consensus/replay_test.go

76f28ad

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

refactor: changes according to PR feedback

9d3810c

fix: blocking wal_test.go because of default context

b2f1b01

Merge remote-tracking branch 'origin/same-block-execution-stabilizing…

d3ce463

…' into same-block-execution-stabilizing

Merge branch 'proof-of-concept-same-block-execution' into same-block-…

0fee9e4

…execution-stabilizing

fix: code style issues

87c7f1f

fix: abci-cli test

7f6f141

fix: typo in struct name CurentRoundState => CurrentRoundState

a940ae7

fix: a few data race issues

842b3cb

shotonoff commented Sep 7, 2022

View reviewed changes

shotonoff added 3 commits September 7, 2022 12:14

fix: TestHandshakeReplayNone

2d156f5

fix: ValidatorUpdate.MarshalJSON

0438d6d

fix: ValidatorUpdate.UnmarshalJSON

efd27a6

shotonoff requested a review from lklimek September 7, 2022 13:10

lklimek approved these changes Sep 7, 2022

View reviewed changes

chore(e2e): prototype e2e test app for same-block execution

ca907c2

lklimek and others added 7 commits September 7, 2022 17:13

fix(consensus): state is nil on genesis

648ef64

fix(state): invalid stateID height on genesis

4e61e59

chore: improve logging

0aac899

Update abci/tests/server/client.go

cfafdee

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

Update abci/tests/server/client.go

54207b2

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

Update internal/proxy/client_test.go

156431b

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>

refactor: changes according to PR feedback

54efd9e

shotonoff merged commit 74c0aac into proof-of-concept-same-block-execution Sep 8, 2022

shotonoff deleted the same-block-execution-stabilizing branch September 8, 2022 07:57

Conversation

shotonoff commented Sep 1, 2022

Issue being fixed or feature implemented

What was done?

How Has This Been Tested?

Breaking Changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shotonoff left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants