Align specs with base Tendermint and current lazyledger-core impl #160

adlerjohn · 2021-05-10T17:51:27Z

Fixes #139. (Explicitly does not address #153 or #154).

Aligns specs with the current data structures (and implied consensus rules) in https://github.com/lazyledger/lazyledger-core.

liamsi · 2021-05-10T18:26:42Z

rationale/fees.md

@@ -6,6 +6,8 @@

 ## Preamble

+**This document will apply to a future version of the LazyLedger protocol.**


What we could do: we could tag the current state of the spec (v0.1.0-idealized) and remove all things that we plan to add later. That might be clearer than this and we have a tagged version that we can refer to in future discussions?

We should be tagging this repo eventually, but for now we can honestly just delete this file completely (it can always be referenced by commit hash).

For future ref: https://github.com/lazyledger/lazyledger-specs/blob/066e14fca9de22555abc70dd4bcf4017fd0bfc64/rationale/fees.md

(it can always be referenced by commit hash).

Sure, but I feel like adding a tag is easier to find and more explicit.

Fixed in fb2d892.

specs/consensus.md

liamsi · 2021-05-10T20:27:03Z

Fixes #139. (Explicitly does not address #153 or #154).

Not really about #139 though right? see:

We should open issues in lazyledger-core with the changes to core types.

Or are you saying that there are no changes left that need to be done to core types? I think there still are a few places in ll-core.

So the changes I see:

height, round -> int instead of uint
use Timestamp instead
remove FeeHeader and rationale doc about fees
Version (single int) -> ConsensusVersion struct
CHAIN_ID -> string

Is there anything else?

BTW, 1., 2. were completely uncontroversial changes. I think it's still good to reset them but we should track these somewhere as desired and easy changes before launch.

adlerjohn · 2021-05-10T22:59:22Z

Or are you saying that there are no changes left that need to be done to core types? I think there still are a few places in ll-core.

It's more that, with this PR, there are no more "big" changes that need to be made (e.g. that require changes to ABCI), or superfluous changes that aren't critical for functionality. The (very few remaining) changes that do need to be made are in the process of being taken care of in the ll-core repo, so #139 is no longer really relevant. Making improvements to the contributing policy is superseded by #158.

Is there anything else?

There's also

Re-adding version and chain ID to the header.
Changing consensus root to consensus hash.

Wondertan · 2021-05-11T12:24:49Z

specs/data_structures.md

+| `Amount`                    | `uint64`                    |
+| `Graffiti`                  | `byte[MAX_GRAFFITI_BYTES]`  |
+| [`HashDigest`](#hashdigest) | `byte[32]`                  |
+| `Height`                    | `int64`                     |


int64 vs uint64? Idk exactly

If there is no difference, changing the specs to conform impl is easier. Otherwise, I can change the impl, like I did in #315 also if uin64 is in some way better.
Same for Round

Wondertan · 2021-05-11T12:29:22Z

specs/proto/consensus.proto

-  uint64 available_data_original_shares_used = 9;
-  AvailableDataHeader available_data_header = 10;
+  bytes state_commitment = 7;
+  uint64 available_data_original_shares_used = 8;


The thing is that I just changed this value in the impl, but now we change all other unsigned integers leaving this one of its kind and losing consistency. Let's decide once and for all on what type of integers we use and keep the same pattern everywhere changing impl/specs if necessary. In places where there is a specific reason for integer type we can add a commit

/cc @liamsi

The purpose of this PR is to describe the current state of the implementation too. I think you are right about the uints (but it is still orthogonal). I would also have left the uints because they are uncontroversial. That's why I've also mentioned above:

BTW, 1., 2. were completely uncontroversial changes. I think it's still good to reset them but we should track these somewhere as desired and easy changes before launch.

Consistency for all integers is not really that important (and not urgent) though. But in general I think we should change to uints. Wer can think about this in another iteration. Let's first have the spec and implementation more aligned first though. More important is fixing #153 and #154.

Maybe then a new issue is needed for integer consistency?

As @liamsi, this PR is to align specs with current implementation, not to be ideal. Going forwards, changes in the specs to these types should be accompanied by an implementation or at least an ADR, and changes to the implementation should be accompanied by a change to the specs.

But you're right that eventually we want to use uint instead of ints, and everything should be consistent. The implication is that this whole PR will be reverted in due time.

The implication is that this whole PR will be reverted in due time.

That was no clear for me, ok

Align specs with base Tendermint and current lazyledger-core impl.

c2e6846

adlerjohn added the consensus label May 10, 2021

adlerjohn self-assigned this May 10, 2021

adlerjohn added 3 commits May 10, 2021 14:12

Fix proto.

caeb565

Fix proto types.

fc5d095

Fix ToC.

066e14f

adlerjohn marked this pull request as ready for review May 10, 2021 18:17

adlerjohn requested review from evan-forbes, liamsi and Wondertan May 10, 2021 18:17

liamsi reviewed May 10, 2021

View reviewed changes

specs/consensus.md Show resolved Hide resolved

Remove fee burn rationale document.

fb2d892

adlerjohn changed the title ~~Align specs with base Tendermint and current lazyledger-core impl.~~ Align specs with base Tendermint and current lazyledger-core impl May 10, 2021

adlerjohn requested a review from liamsi May 10, 2021 19:58

liamsi approved these changes May 11, 2021

View reviewed changes

Wondertan reviewed May 11, 2021

View reviewed changes

adlerjohn merged commit bfd7d00 into master May 11, 2021

adlerjohn deleted the adlerjohn/align_with_base_tendermint branch May 11, 2021 15:56

adlerjohn mentioned this pull request May 29, 2021

Update data structures figure #166

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Align specs with base Tendermint and current lazyledger-core impl #160

Align specs with base Tendermint and current lazyledger-core impl #160

adlerjohn commented May 10, 2021 •

edited

Loading

liamsi May 10, 2021

adlerjohn May 10, 2021 •

edited

Loading

adlerjohn May 10, 2021

liamsi May 10, 2021

adlerjohn May 10, 2021

liamsi commented May 10, 2021 •

edited

Loading

adlerjohn commented May 10, 2021 •

edited

Loading

Wondertan May 11, 2021 •

edited

Loading

Wondertan May 11, 2021

liamsi May 11, 2021

Wondertan May 11, 2021

adlerjohn May 11, 2021 •

edited

Loading

Wondertan May 11, 2021 •

edited

Loading

		@@ -6,6 +6,8 @@

		## Preamble

		This document will apply to a future version of the LazyLedger protocol.

Align specs with base Tendermint and current lazyledger-core impl #160

Align specs with base Tendermint and current lazyledger-core impl #160

Conversation

adlerjohn commented May 10, 2021 • edited Loading

liamsi May 10, 2021

Choose a reason for hiding this comment

adlerjohn May 10, 2021 • edited Loading

Choose a reason for hiding this comment

adlerjohn May 10, 2021

Choose a reason for hiding this comment

liamsi May 10, 2021

Choose a reason for hiding this comment

adlerjohn May 10, 2021

Choose a reason for hiding this comment

liamsi commented May 10, 2021 • edited Loading

adlerjohn commented May 10, 2021 • edited Loading

Wondertan May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Wondertan May 11, 2021

Choose a reason for hiding this comment

liamsi May 11, 2021

Choose a reason for hiding this comment

Wondertan May 11, 2021

Choose a reason for hiding this comment

adlerjohn May 11, 2021 • edited Loading

Choose a reason for hiding this comment

Wondertan May 11, 2021 • edited Loading

Choose a reason for hiding this comment

adlerjohn commented May 10, 2021 •

edited

Loading

adlerjohn May 10, 2021 •

edited

Loading

liamsi commented May 10, 2021 •

edited

Loading

adlerjohn commented May 10, 2021 •

edited

Loading

Wondertan May 11, 2021 •

edited

Loading

adlerjohn May 11, 2021 •

edited

Loading

Wondertan May 11, 2021 •

edited

Loading