Refactor submission bitcoin status #170

KonradStaniec · 2022-10-15T13:06:15Z

Simplifies btccheckpoint code, by getting rid of different methods checking for submission bitcoin status.

vitsalis

Very cool simplification! Some comments

vitsalis · 2022-10-17T07:57:32Z

x/btccheckpoint/keeper/keeper.go

-	}
+		if err != nil {
+			// one of blocks is not known to light client
+			return OnFork, err


Since OnFork implies that something exists on btc, maybe we can have an extra code Unknown or similar for demonstrating this use case.

The thing is we do not handle in any meaningful way submissions which are in blocks not known to lightclient.

They:

are not accepted when submitted

the should be deleted if they ware accepted, and then forgotten by lightclient (this means they ended on btc fork, and headers were pruned)

Therefore imo it is just one enum case more which we would need to maintain in some way but would not be in any case useful. Thats why it is treated as error.

vitsalis · 2022-10-17T07:59:53Z

x/btccheckpoint/keeper/keeper.go

-	// counts as unconfirmed submission.
+	// always start submission lifecycle from unconfirmed state, even if it is
+	// confirmed or finalized. It will quickly reach next states with btc
+	// light client blocks
 	k.addToUnconfirmed(ctx, sk)


Maybe we can name this addToSubmitted to ensure consistency on names. Also, couldn't we use the submissionBtcState to identify in which state we can add this? I understand that this will finally reach the correct state, but why should we wait for a new BTC block to update the status?

Sure will re-name it 👍

As for second point, we could probably use submissionBtcState to jump directly to correct state, but:

It nicer to have every submission to go to through correct state transition path i.e Submitted -> Confirmed -> Finalized

Jumping to correct state here would probably means we need more special logic here i.e should be check if this submission confirm/finalised whole epoch, should be clear other submissions if epoch state changes etc. Therefore it is simpler to just treat every submission as submitted at the begnining.

The price for it is of course, that it can decrease latency a bit as we need movement in btclightclient, but it should not be a big deal.

vitsalis · 2022-10-17T08:02:45Z

x/btccheckpoint/keeper/keeper.go

 	}

-	return uint64(depth), true, nil
+	if submissionDepth >= k.GetParams(ctx).CheckpointFinalizationTimeout {


Suppose that the submission is split between blocks N and N+1, and we are at height N+w. Then the submission will be treated as finalized, but block N+1 is not w-deep. Do we consider this acceptable?

Not really, it the loop above this code, we calculate submission depth as the depth of its most fresh header. (header with lower depth)

So if one header has depth 10 and another header has depth 20 then depth of submission is 10 so if our confirmation depth is 15, then it that case submission won't be confirmed.

Indeed, thanks for explaining

vitsalis · 2022-10-17T08:09:51Z

x/btccheckpoint/keeper/msg_server.go

+		)
+
+		if err != nil {
+			panic("Headers which are should have been known to btclight client")


Headers which are should ?

vitsalis · 2022-10-17T08:14:58Z

x/btccheckpoint/keeper/msg_server.go

+	// TODO: Either get rid of accepting subbmisions on forks, or design some better
+	// mechanism to deal with that case
+	if !rawSubmission.InOneBlock() && submissionState == OnFork {
+		onTheSameFork, err := m.onTheSameFork(


While on other methods we treat the number of submissions as variable (e.g. the GetSubmissionBtcState function), here we consider that there are two of those. Maybe we can abstract this inside the onTheSameFork function and just pass rawSubmission as an argument.

gitferry

Nice refactoring! Left some small comments.

gitferry · 2022-10-17T11:14:56Z

x/btccheckpoint/keeper/keeper.go

+)
+
+const (
+	Submitted SubmissionBtcState = iota


Would SubmissionBtcStatus be a better name? Just to be consistent.

gitferry · 2022-10-17T11:26:59Z

x/btccheckpoint/keeper/msg_server.go

+	// is implicit.
+	// TODO: Either get rid of accepting subbmisions on forks, or design some better
+	// mechanism to deal with that case
+	if !rawSubmission.InOneBlock() && submissionState == OnFork {


If they are in one block, which is OnFork, why not reject the submission?

because it can in principle end up in main chain.

My position in general is that we should not accept submission on forks at all (https://babylon-chain.atlassian.net/wiki/spaces/BABYLON/pages/34504933/ADR-001+Checkpoint+submission+acceptance+rules) and in general get rid of them as soon as possible (I think I am still waiting on @fishermanymc input on this)

If this propsal will be accepted I will make another pr which will further simplify a lot of logic here.

vitsalis

Thanks for the updates!

* Update babylon-contract * Update README * Fix localnet nodes 3 & 4 * Fix babylon contract instantiation * Fix / adapt contract execution test * Contract download script (#155) Add download contract from release script * Fix typos * Reduce network / IB relayer start wait time * Move IBC config to chain config * Add BTC Timestamping Phase 2 test * Improve syntax Co-authored-by: Runchao Han <[email protected]> * Revert "Fix localnet nodes 3 & 4" This reverts commit aaf3707b28985d5f03261107d0fad518fc478527. * Now fix localnet nodes 3 & 4 * Simplify phase-2 vs phase-1 test setup * Simplify phase 2 test * Make chain-B the CZ chain * Use regtest const for network * Use default checkpoint tag const for babylon tag Update contract to custom * Fix comment Co-authored-by: Runchao Han <[email protected]> * Update contract to custom * Fix original test as well * Increase delay to wait for chain ready * Add (failing) next IBC packet sequence check * Fix: contains wasm check * Storage contract e2e (#171) * Revert "Update babylon-contract" This reverts commit 7c656b183818098f2662ac9996cde80b8f4009ba. * Update storage-contract to custom * Revert "Update README" This reverts commit 3669c88d16b2ea1c8185ed53a74be0a0ae3b11ea. * Revert "Fix babylon contract instantiation" This reverts commit 1fb44d7fcfac3d79a4c8b9323a56eb8b0a837d83. * Revert "Fix / adapt contract execution test" This reverts commit 124099dd64dd519aba0774d3e5a7e43d7a44162a. * Fix imports * fix phase-2 e2e test (#163) * Upgrade keyring / go-keychain dependency (#170) * Add cosmos relayer docker image build * Reduce cosmos-relayer image * Set / use relayer tag * Improve hermes bootstrap script * Set ABCI packet persistence * Add Cosmos relayer setup * Enable debug logs * Reduce relayer setup wait time * Simplify seup / remove commented lines * fix * Fix: packet acknowledgements check * Rename for consistency * Same acknowledgements logic for Hermes relayer * More test renaming for consistency * Skip BTC timestamping phase 2 test (Hermes) * Fix: BTC timestamping phase 1 tests * Improve flaky BTC timestamping tests * Return network to RegTest * Update contracts code to latest * Remove docker Platform directive / commented code * Increase wait for blocks delay * Add cosmos relayer docker build target to e2e target deps * Fix rebase errors * Remove unused helper * Add TODOs * Remove commented code * Comment BTC timestamping hermes test out * Restore original BTC timestamping test * Report target arch / Go lang version * Hardcode target arch for CI * Hardcode next expected sequence --------- Co-authored-by: Runchao Han <[email protected]> Co-authored-by: Mauro Lacy <[email protected]>

KonradStaniec requested review from vitsalis, gitferry and SebastianElvis October 15, 2022 13:06

KonradStaniec force-pushed the simplify-handling-of-btc-status branch from e6a194a to ccbe6e8 Compare October 16, 2022 11:08

vitsalis reviewed Oct 17, 2022

View reviewed changes

gitferry reviewed Oct 17, 2022

View reviewed changes

KonradStaniec added 2 commits October 17, 2022 19:19

Refactor submission bitcoin status

87cccfa

Pr comments

319ca72

KonradStaniec force-pushed the simplify-handling-of-btc-status branch from 108ac55 to 319ca72 Compare October 17, 2022 17:19

vitsalis approved these changes Oct 18, 2022

View reviewed changes

KonradStaniec merged commit ae35adf into main Oct 18, 2022

KonradStaniec deleted the simplify-handling-of-btc-status branch October 18, 2022 09:42

vitsalis pushed a commit that referenced this pull request Jan 21, 2024

Upgrade keyring / go-keychain dependency (#170)

7282d2d

maurolacy added a commit that referenced this pull request Jan 22, 2024

Upgrade keyring / go-keychain dependency (#170)

cf38bf7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor submission bitcoin status #170

Refactor submission bitcoin status #170

KonradStaniec commented Oct 15, 2022

vitsalis left a comment

vitsalis Oct 17, 2022

KonradStaniec Oct 17, 2022

vitsalis Oct 17, 2022

KonradStaniec Oct 17, 2022 •

edited

Loading

vitsalis Oct 17, 2022

KonradStaniec Oct 17, 2022

vitsalis Oct 18, 2022

vitsalis Oct 17, 2022

vitsalis Oct 17, 2022

gitferry left a comment

gitferry Oct 17, 2022

KonradStaniec Oct 17, 2022

gitferry Oct 17, 2022

KonradStaniec Oct 17, 2022 •

edited

Loading

vitsalis left a comment

Refactor submission bitcoin status #170

Refactor submission bitcoin status #170

Conversation

KonradStaniec commented Oct 15, 2022

vitsalis 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

KonradStaniec Oct 17, 2022 • edited Loading

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

gitferry 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

KonradStaniec Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

vitsalis left a comment

Choose a reason for hiding this comment

KonradStaniec Oct 17, 2022 •

edited

Loading

KonradStaniec Oct 17, 2022 •

edited

Loading