feat: retrieves timeout propose and timeout commit dynamically per height according to the app version #1494

staheri14 · 2024-09-19T17:26:55Z

The celestia-core portion of #3859

…s-timeouts-to-endblock-abci

…and vice versa

evan-forbes

for future readers, we've had discussions on if we need to store the timeouts in the state or not. We don't need to, as we're always capable of reading these values from the application. The rpc portion was also only added for being able to query nodes during tests, but is not needed.

also per a previous discussion, we don't need to block on that.

will approve after some e2e testing, either here or in the knuu tests

remove block

evan-forbes

👍 thanks for testing this

its not a major deal, but in the future I think we can make reviewer's lives easier by limiting the scope of PRs. there are a lot of changes here such as unrelated spelling / doc stuff and RPC / storing the state that I think would have been possible to avoid.

if there are any remaining todos for edge cases or cleanups, then do you mind writing those up @staheri14 sometime next week?

staheri14 · 2024-10-11T22:41:04Z

its not a major deal, but in the future I think we can make reviewer's lives easier by limiting the scope of PRs. there are a lot of changes here such as unrelated spelling / doc stuff and RPC / storing the state that I think would have been possible to avoid.

I agree, I myself am against large PRs. However, for this one, I initially suggested creating an ADR, but it was deemed unnecessary. If we had an ADR, then major design decisions could have been documented, followed by smaller PRs based on that design. Since there was no design document, I had to include everything in a single PR to ensure it made sense to the reviewer. But if it makes it easier to review, I can and will be willing to split the PR into smaller PRs.

staheri14 · 2024-10-11T23:17:17Z

if there are any remaining todos for edge cases or cleanups, then do you mind writing those up @staheri14 sometime next week?

Sure, will do next week!

config/config.go

cmwaters · 2024-10-14T13:24:46Z

consensus/state.go

@@ -674,9 +674,28 @@ func (cs *State) updateToState(state sm.State) {
 		// to be gathered for the first block.
 		// And alternative solution that relies on clocks:
 		// cs.StartTime = state.LastBlockTime.Add(timeoutCommit)
-		cs.StartTime = cs.config.Commit(cmttime.Now())
+
+		if state.LastBlockHeight == 0 {


If you want to keep this can you compact this into a simpler IsGenesis method and then just have a single if statement to set cs.StartTime

state/execution.go

state/store.go

cmwaters · 2024-10-14T14:45:18Z

Thanks for clarifying that saving timeouts and the RPC endpoint are for the celestia-app e2e tests. I had initially thought they were used in this repo for testing. My approach would be to measure the block time before the upgrade and measure the block time after the upgrade and assert that the first is close to 11 seconds and the second is close to 6 seconds

…estiaorg/celestia-core into sanaz/adds-timeouts-to-endblock-abci

rootulp

A lot of the comments I left on the last review haven't been addressed. Do you want me to continue re-reviewing?

rootulp · 2024-10-14T17:23:25Z

consensus/state.go

+			// state represents the genesis state, hence the StartTime should be set based on the state's TimeoutCommit
+			// we don't use cs.state.TimeoutCommit because that is zero
+			cs.StartTime = cs.config.CommitWithCustomTimeout(cmttime.Now(), state.TimeoutCommit)
+		} else {
+			// if state does not represent the genesis state,
+			// we use the cs.state.TimeoutCommit
+			cs.StartTime = cs.config.CommitWithCustomTimeout(cmttime.Now(), cs.state.TimeoutCommit)
+		}


This comment wasn't addressed.

rootulp · 2024-10-14T17:24:25Z

node/node.go

+	fastSync := config.FastSyncMode && !onlyValidatorIsUs(state,
+		pubKey) // the state certainly has the latest timeouts according


This comment wasn't addressed.

rootulp · 2024-10-14T17:24:38Z

node/node.go

+	// app may modify the validator set, specifying ourselves as the only validator.
+	fastSync := config.FastSyncMode && !onlyValidatorIsUs(state,
+		pubKey) // the state certainly has the latest timeouts according
+	// the app version lastBlock in the blockstore


This comment wasn't addressed.

rootulp · 2024-10-14T17:24:49Z

node/node.go

@@ -888,6 +895,7 @@ func NewNodeWithContext(ctx context.Context,
 	}

 	// Make MempoolReactor
+	// TODO limit the proxyApp to Mempool only


This comment wasn't addressed.

rootulp · 2024-10-14T17:24:56Z

node/node.go

@@ -943,6 +952,7 @@ func NewNodeWithContext(ctx context.Context,
 	}

 	// Setup Transport.
+	// TODO limit the proxyApp to Query only


This comment wasn't addressed.

rootulp · 2024-10-14T17:25:42Z

proto/tendermint/state/types.proto

@@ -77,4 +77,7 @@ message State {

  // the latest AppHash we've received from calling abci.Commit()
  bytes app_hash = 13;
+
+  // timeouts to be used for the next block height
+  tendermint.abci.TimeoutsInfo timeouts = 15 [(gogoproto.nullable) = false];


[question] why not 14? The last used field number is 13.

this is a good question

It's weirdly formatted but above there is initial_height which uses the 14 tag

rootulp · 2024-10-14T17:26:02Z

rpc/core/routes.go

@@ -54,6 +54,9 @@ var Routes = map[string]*rpc.RPCFunc{

 	// evidence API
 	"broadcast_evidence": rpc.NewRPCFunc(BroadcastEvidence, "evidence"),
+
+	// get Timeouts by height
+	"timeout": rpc.NewRPCFunc(ConsensusTimeoutsInfo, "height"),


This comment wasn't addressed.

evan-forbes · 2024-10-14T17:31:04Z

@rootulp @cmwaters

just removed rpc and storing the timeouts, and most importantly, set the timeouts after state sync after the call to Info

other nits with comments and spelling etc were handled in the first two commits, but I'm going through now to make sure I got all of them

evan-forbes · 2024-10-14T17:31:28Z

I'll still need to fix the linter, so there will be at least one more commit

rootulp

A few of my comments still haven't been addressed but not going to block on any of them.

golangci-lint is failing.

It would have been nice if the PR description included a Testing section that describes how this PR was manually tested.

state/store.go

…ight according to the app version (#1494) The celestia-core portion of [#3859](celestiaorg/celestia-app#3859) --------- Co-authored-by: evan-forbes <[email protected]>

staheri14 added 6 commits September 18, 2024 13:10

extends ResponseEndBlock protobuf message with timeouts

f231895

reads the timeouts and attempts to set them

41a2cb0

extends state struct with timeout fields

1fc1edf

updates timeouts based on EndBlockResponse

597f667

updates consensus configs based on ApplyBlock's results

2397315

changes the type of timeout fields in protobuf to uint64

03fb213

staheri14 changed the title ~~WIP~~ WIP: embeds timeouts in abci calls Sep 19, 2024

uses time.Duration for protobuf fields

a104580

staheri14 self-assigned this Sep 19, 2024

staheri14 added 21 commits September 19, 2024 14:52

adds timeout fields to RequestInfo

30bc1c0

consolidates timeouts inside one protobuf message type

7b3618b

moves tiemouts to the response msg

fa27abe

adds timemouts to InitchainRespo se

6e2fb78

corrects some of the docs

a7d1e1b

remnants of adding timeouts to the InitchainResponse

0904cd5

disambiguates a comment

0271370

minor edits

47329db

adds some todos

8ef6666

fixes godocs

6a92b18

adds timeouts to the State protobuf message to be saved in the statedb

782efa1

fixes godocs

26dfedf

Merge remote-tracking branch 'origin/v0.34.x-celestia' into sanaz/add…

020efac

…s-timeouts-to-endblock-abci

makes timeouts field not nullable

490189c

exposes Timeout fields of the State

852d07f

copies over timeout values when converting from proto to local state …

f1568a6

…and vice versa

tests copying timeouts

00d8e1c

extends TestStateSaveLoad to account for timeout equality

3b6cbb3

adds a clarifying comment

98feed8

integrates timeouts in TestABCIResponsesSaveLoad1

9ecf2d1

sets non-zero timeouts for the TestStateProto

063fafd

staheri14 requested a review from rootulp October 11, 2024 21:02

clarifies what default means

85e92f3

evan-forbes reviewed Oct 11, 2024

View reviewed changes

evan-forbes previously approved these changes Oct 11, 2024

View reviewed changes

staheri14 mentioned this pull request Oct 11, 2024

feat!: versioned timeouts celestiaorg/celestia-app#3882

Merged

cmwaters reviewed Oct 14, 2024

View reviewed changes

evan-forbes added 5 commits October 14, 2024 11:53

chore: comment feedback @rootulp

5d399b5

chore: review feedback

081ecce

chore: remove rpc and timeouts from the state store

f12da04

fix: use the timeouts passed from info

12639d4

Merge branch 'sanaz/adds-timeouts-to-endblock-abci' of github.com:cel…

e772715

…estiaorg/celestia-core into sanaz/adds-timeouts-to-endblock-abci

evan-forbes dismissed their stale review via e772715 October 14, 2024 17:25

rootulp reviewed Oct 14, 2024

View reviewed changes

rootulp self-requested a review October 14, 2024 17:30

rootulp previously approved these changes Oct 14, 2024

View reviewed changes

state/store.go Outdated Show resolved Hide resolved

chore: linter

1ef98d0

evan-forbes dismissed rootulp’s stale review via 1ef98d0 October 14, 2024 17:35

chore: remaining two comments

23e3fb1

rootulp approved these changes Oct 14, 2024

View reviewed changes

evan-forbes enabled auto-merge (squash) October 14, 2024 17:53

evan-forbes approved these changes Oct 15, 2024

View reviewed changes

evan-forbes merged commit c2c6463 into v0.34.x-celestia Oct 15, 2024
18 checks passed

evan-forbes deleted the sanaz/adds-timeouts-to-endblock-abci branch October 15, 2024 01:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: retrieves timeout propose and timeout commit dynamically per height according to the app version #1494

feat: retrieves timeout propose and timeout commit dynamically per height according to the app version #1494

staheri14 commented Sep 19, 2024 •

edited

Loading

evan-forbes left a comment

evan-forbes left a comment •

edited

Loading

staheri14 commented Oct 11, 2024 •

edited

Loading

staheri14 commented Oct 11, 2024

cmwaters Oct 14, 2024

cmwaters commented Oct 14, 2024

rootulp left a comment

rootulp Oct 14, 2024

rootulp Oct 14, 2024

rootulp Oct 14, 2024

rootulp Oct 14, 2024

rootulp Oct 14, 2024

rootulp Oct 14, 2024

evan-forbes Oct 14, 2024

cmwaters Oct 15, 2024

rootulp Oct 14, 2024

evan-forbes commented Oct 14, 2024

evan-forbes commented Oct 14, 2024

rootulp left a comment

		fastSync := config.FastSyncMode && !onlyValidatorIsUs(state,
		pubKey) // the state certainly has the latest timeouts according

feat: retrieves timeout propose and timeout commit dynamically per height according to the app version #1494

feat: retrieves timeout propose and timeout commit dynamically per height according to the app version #1494

Conversation

staheri14 commented Sep 19, 2024 • edited Loading

evan-forbes left a comment

Choose a reason for hiding this comment

evan-forbes left a comment • edited Loading

Choose a reason for hiding this comment

staheri14 commented Oct 11, 2024 • edited Loading

staheri14 commented Oct 11, 2024

Choose a reason for hiding this comment

cmwaters commented Oct 14, 2024

rootulp 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

evan-forbes commented Oct 14, 2024

evan-forbes commented Oct 14, 2024

rootulp left a comment

Choose a reason for hiding this comment

staheri14 commented Sep 19, 2024 •

edited

Loading

evan-forbes left a comment •

edited

Loading

staheri14 commented Oct 11, 2024 •

edited

Loading