docs: ADR-017 single binary sync #1521

mojtaba-esk · 2023-03-22T15:45:36Z

Overview

Checklist

New and updated code has appropriate documentation
New and updated code has new and/or updated testing
Required CI checks are passing
Visual proof for any user facing features like CLI or documentation updates
Linked issues closed with keywords

github-actions · 2023-03-22T15:45:55Z

PR Preview Action v1.3.0
🚀 Deployed preview to https://celestiaorg.github.io/celestia-app/pr-preview/pr-1521/
on branch `gh-pages` at 2023-03-22 15:45 UTC

evan-forbes · 2023-03-22T15:51:58Z

is this a draft?

rootulp · 2023-03-23T09:36:35Z

docs/architecture/adr-017-single-binary-upgrade.md

+
+It important to sync from genesis with one binary.
+It is needed for wallets, block explorers, indexers, analytics (e.g. Nansen).
+It makes it possible to sync headers from chain and download blobs from third party service.


I think we need to clarify this. In the meeting the point was made that the Celestia network doesn't need to retain all historical blob data and that blobs could eventually be pruned and fetched from a third party service.

I don't see how single binary sync inherently makes this possible. Maybe this can state that an engineering requirement is that a single celestia-app binary can fetch all historical block headers from the chain (?)

I think might be able to modify this a bit to discuss the added functionality, which is that a single binary is capable of verifying the entire chain. Syncing using a single binary ideally also makes it easy to maintain all of the different binaries, so that node operators can do that in a safe way.

ideally, the first part of this document should provide the context of why, and a summarizing some of the arguments or approaches for the rest of the document.

bumping this 🙂

docs/architecture/adr-017-single-binary-upgrade.md

rootulp · 2023-03-23T09:38:51Z

docs/architecture/adr-017-single-binary-upgrade.md

+
+Instead of having a single binary, we can have multiple binaries that are managed by another supervisory binary e.g. [Cosmovisor](https://docs.desmos.network/fullnode/cosmovisor/).
+Osmosis does it through having a significant number of binaries gathered in one docker container and switch them in particular heights.
+This approach has a lot of maintenance burden and is fragile to sync from genesis.


This approach has a lot of maintenance burden

I'm not convinced that the Osmosis approach has any more maintenance burden than what we're proposing with this ADR.

and is fragile to sync from genesis

Are there scenarios where Osmosis sync from genesis fails? Should we include data to support this point?

docs/architecture/adr-017-single-binary-upgrade.md

rootulp · 2023-03-23T09:45:16Z

docs/architecture/adr-017-single-binary-upgrade.md

+- UX issue to improve experience for node operators to upgrade
+
+- Maintenance issue for maintaining historical releases of binaries


Maybe we can articulate these as the two compelling points driving this ADR. So proposal to life them to the top as part of a problem statement section

rootulp · 2023-03-23T09:46:40Z

docs/architecture/adr-017-single-binary-upgrade.md

+- What granularity should conditional logic be?
+  - App struct level
+  - Module level
+  - if/else on a line by line basis
+
+- Why app version and not block height?
+  - App version is bumped via upgrade module
+  - Depending on app version is universal across multiple networks.
+  - Block height will differ across testnets / chain-ids so depends on a `.toml` config file
+
+- Are we planning on upstream this?
+  - [CometBFT](https://github.com/cometbft/cometbft) is working on this for consensus.
+  - We’re working on Cosmos SDK portion
+
+- `celestia-node` light nodes need to version erasure coding block or celestia-node should import this from celestia-app
+
+- Will celestia-node need to pass a block height to celestia-app? Header contains version.app (state machine version) and version.block (consensus version)
+
+- Is it important to support rolling back versions? Involves upgrading (again) through the faulty version
+
+- Since if conditional logic uses one Go version, all code will be compiled with this one Go version. If behavior changes across Go versions, we have to debug why.
+
+- Option: depend on multiple major versions of a dependency. What is the relationship between if/else conditional statements and Cosmos SDK upstream changes?
+
+- How to do this in CometBFT?
+  - Always use latest p2p version
+  - Version.block in header should be used to version changes to Tendermint / CometBFT
+
+- Block sync needs to be aware of changes across block heights


I'm not sure if these raw notes make sense to include in the ADR but defer to your judgement

rootulp · 2023-03-23T09:52:14Z

docs/architecture/adr-017-single-binary-upgrade.md

+}
+```
+
+This option has an advantage over the option #1 as we do not need to pass an extra parameter everywhere in the code and it has less maintenance burden to modify the version logic.


A counterpoint is that option 2 makes it more difficult to unit test because pure functions are easier to test than impure functions.

Implementations in option 2 rely on a global state. It is likely still possible to unit test by mocking the version module with a test version module but I think it makes sense to call this out as a downside.

Related question: how do we ensure all necessary packages that want to read the global version will invoke it after it has been set?

docs/architecture/adr-017-single-binary-upgrade.md

rootulp · 2023-03-23T09:53:40Z

docs/architecture/adr-017-single-binary-upgrade.md

+There are multiple ways of doing it. The app version can be found in the SDK context.
+So, in the `PrepareProposal` method, we can get the app version [from the context](https://github.com/celestiaorg/celestia-app/blob/main/app/prepare_proposal.go#L28) and use it as the version indicator.
+
+#### Option 1: Passing the app version throughout all the required functions


evan-forbes

great start!!

the initial notes are a really good start, but we should take that information and construct it into a more cohesive ADR document.

Co-authored-by: Rootul P <[email protected]>

evan-forbes

we still need to flesh this out a bit by providing code examples of using the app version and discussing a few of the edge cases, but I think we're on the right track.

we should also convert this to an ADR and remove any notes that are not relevant to the discussion/decision when we get the chance 🙂

evan-forbes · 2023-04-03T10:37:49Z

docs/architecture/adr-017-single-binary-upgrade.md

+#### Option 1: Passing the app version throughout all the required functions
+
+In this case we need to pass either sdk context or the app version itself to all the functions and objects that we think they might be affected by a version change in future.
+So for those functions that do not have the ctx already, we need to add an extra parameter to pass it.


which functions do not have access to the app version? I believe just PrepareProposal, as the header is passed to ProcessProposal

evan-forbes · 2023-04-03T10:38:43Z

docs/architecture/adr-017-single-binary-upgrade.md

+func mergeMaps(mapOne, mapTwo map[coretypes.TxKey]ShareRange) map[coretypes.TxKey]ShareRange {
+	merged := make(map[coretypes.TxKey]ShareRange, len(mapOne)+len(mapTwo))


what is ShareRange here?

evan-forbes · 2023-04-03T10:41:39Z

docs/architecture/adr-017-single-binary-upgrade.md

+
+## Decision
+
+TBD


let's update this to using the app version and arbitrary switch statements

evan-forbes · 2023-04-03T10:42:11Z

docs/architecture/adr-017-single-binary-upgrade.md

+
+It important to sync from genesis with one binary.
+It is needed for wallets, block explorers, indexers, analytics (e.g. Nansen).
+It makes it possible to sync headers from chain and download blobs from third party service.


bumping this 🙂

evan-forbes · 2023-04-03T10:43:18Z

docs/architecture/adr-017-single-binary-upgrade.md

+- What granularity should conditional logic be?
+  - App struct level
+  - Module level
+  - if/else on a line by line basis
+
+- Why app version and not block height?
+  - App version is bumped via upgrade module
+  - Depending on app version is universal across multiple networks.
+  - Block height will differ across testnets / chain-ids so depends on a `.toml` config file
+
+- Are we planning on upstream this?
+  - [CometBFT](https://github.com/cometbft/cometbft) is working on this for consensus.
+  - We’re working on Cosmos SDK portion


are these just notes from the call? if so, perhaps we should clean this up and focus on the content of this ADR before merging

evan-forbes · 2023-09-13T17:42:33Z

closing for now in favor of #1562

Draft of the ADR 017 added

63e35bb

mojtaba-esk requested review from liamsi, adlerjohn, MSevey, evan-forbes and rootulp as code owners March 22, 2023 15:45

MSevey requested a review from a team March 22, 2023 15:45

rootulp changed the title ~~docs:adr-017 Draft of the ADR for single binary upgrade added~~ docs: ADR-017 single binary sync Mar 22, 2023

evan-forbes marked this pull request as draft March 23, 2023 08:50

some psudo code added

a816112

rootulp reviewed Mar 23, 2023

View reviewed changes

evan-forbes reviewed Mar 23, 2023

View reviewed changes

mojtaba-esk and others added 5 commits March 27, 2023 17:06

docs: genesis init added for the app version

6d6d09f

Update docs/architecture/adr-017-single-binary-upgrade.md

94a4d4c

Co-authored-by: Rootul P <[email protected]>

Update docs/architecture/adr-017-single-binary-upgrade.md

80b392d

Co-authored-by: Rootul P <[email protected]>

Update docs/architecture/adr-017-single-binary-upgrade.md

45cabeb

Co-authored-by: Rootul P <[email protected]>

Update docs/architecture/adr-017-single-binary-upgrade.md

02399c6

Co-authored-by: Rootul P <[email protected]>

rootulp assigned mojtaba-esk Mar 29, 2023

rootulp added the ADR item is directly relevant to writing or modifying an ADR label Mar 29, 2023

This was referenced Mar 30, 2023

EPIC: Social Upgrades #1014

Closed

EPIC: Single Binary Syncs #1568

Closed

evan-forbes reviewed Apr 3, 2023

View reviewed changes

rootulp added this to the Post-mainnet milestone Jul 12, 2023

MSevey removed their request for review July 24, 2023 13:03

evan-forbes closed this Sep 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: ADR-017 single binary sync #1521

docs: ADR-017 single binary sync #1521

mojtaba-esk commented Mar 22, 2023

github-actions bot commented Mar 22, 2023

evan-forbes commented Mar 22, 2023

rootulp Mar 23, 2023

evan-forbes Mar 23, 2023

evan-forbes Apr 3, 2023

rootulp Mar 23, 2023

rootulp Mar 23, 2023

rootulp Mar 23, 2023

rootulp Mar 23, 2023

rootulp Mar 23, 2023

evan-forbes left a comment

evan-forbes left a comment

evan-forbes Apr 3, 2023

evan-forbes Apr 3, 2023

evan-forbes Apr 3, 2023

evan-forbes Apr 3, 2023

evan-forbes Apr 3, 2023

evan-forbes commented Sep 13, 2023

		- UX issue to improve experience for node operators to upgrade

		- Maintenance issue for maintaining historical releases of binaries

		func mergeMaps(mapOne, mapTwo map[coretypes.TxKey]ShareRange) map[coretypes.TxKey]ShareRange {
		merged := make(map[coretypes.TxKey]ShareRange, len(mapOne)+len(mapTwo))


		## Decision

		TBD

docs: ADR-017 single binary sync #1521

docs: ADR-017 single binary sync #1521

Conversation

mojtaba-esk commented Mar 22, 2023

Overview

Checklist

github-actions bot commented Mar 22, 2023

evan-forbes commented Mar 22, 2023

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 left a comment

Choose a reason for hiding this comment

evan-forbes 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

evan-forbes commented Sep 13, 2023