Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft of CAP-0034 (change closetime semantics) #671

Merged
merged 1 commit into from
Jul 9, 2020

Conversation

rokopt
Copy link
Contributor

@rokopt rokopt commented Jul 8, 2020

First draft of CAP-0034, intended to address
stellar-protocol issue #622, by changing the semantics of
the combination of candidates by the SCP ballot protocol
from producing an unsigned form of union of the candidates
(taking, for example, the maximum closetime of all
candidates as the closetime to externalize) to selecting one
particular signed, nominated StellarValue (comprising
transaction set, closetime, and upgrades) as the one to
externalize.

See also the draft implementation.

core/README.md Outdated
@@ -63,6 +63,7 @@
| [CAP-0029](cap-0029.md) | AllowTrust when not AUTH_REQUIRED | Tomer Weller | Draft |
| [CAP-0032](cap-0032.md) | Trustline Preauthorization | Jonathan Jove | Draft |
| [CAP-0033](cap-0033.md) | Sponsored Reserve with EphemeralSponsorshipEntry | Jonathan Jove | FCP |
| [CAP-0034](cap-0034.md) | Single-Candidate Externalized Values | Terence Rokop | Draft |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title doesn't match title in the proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch; fixed.

core/cap-0034.md Outdated
```

## Simple Summary
This CAP proposes to change the way that the nomination protocol generates
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not simple enough. As noted in the template, we're going for a "layman-accessible explanation of the CAP" here. No jargon, no justification, just a simple answer to "what?". Most (but not all) CAP's have just a single sentence here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I got it down to one sentence -- admittedly with three clauses, one parenthetical....

core/cap-0034.md Outdated
throughout the nomination, as the `closeTime` change allows this.

## Motivation
With the current protocol, a transaction with an expiration time might make it
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: This sentence has several nested clauses in it. Could probably be simplified. This comment applies in several places throughout the proposal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried breaking it up into bullet points to see how that reads. I'll look for opportunities to do the same throughout the rest of the document too.

core/cap-0034.md Outdated
number, despite having been delayed long enough to exceed its expiration time.
The fee and sequence number aspects are potentially both problems:

- Transactions submitted by SDKs generally set expiration times so that they
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem totally accurate to me:

  • We recommend time bounds because they give you finality, eg. they all you to determine that a transaction definitely did not and will not execute. You can always submit another transaction with the same sequence number, since at most one of them will execute (you just aren't sure which).
  • If your transaction isn't getting into the transaction set, you're going to use the same sequence number (as it hasn't been consumed) but perhaps a higher fee.

I'm not sure what you mean by the last sentence: fees are only charged if they get included in the transaction set. I suppose you mean that it would make more sense if transactions that had bad time-bounds would not get into the ledger, thereby avoiding charging fees for transactions that are sure to fail?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that section was less than coherent in various places. I reorganized it to place the smart-contract concern first, as it's the most potentially impactful, and I've tried to clarify each individual motivation.

In the course of thinking about it, and trying to clear up my own confusion which led to the confusion of the section's wording, I realized that there was something I still don't understand about the smart-contract problem, which I posted about in the issue.

core/cap-0034.md Outdated
the fee that they were willing to pay that actually got them accepted into a
transaction set.

- Probably worse, there is a design pattern in some smart contracts which
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be more precise about your use of "fail" and "invalid". For example, it is impossible for a transaction to fail with txTOO_EARLY because such a transaction is necessarily invalid when it was submitted.

I also think your example here is unnecessarily complicated. For example, you introduced B but it plays no role in this example. Also it doesn't really matter that the expiration times are in the distant future; the transaction might get submitted on the boundary regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, I've tried to avoid saying "fail" or "invalid" when I meant the other.

Yeah, I noticed that B didn't end up getting used. I thought I had an idea of how it would be, but it turned out superfluous. I've removed it (as well as the speculation about expiration duration and consequent rarity).


### Goals Alignment

This CAP is aligned with the following Stellar Network Goals:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: no need to explain how the goals apply (I would argue that if your proposal truly does align with those goals then the reason should be obvious).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I have left the goals stated unadorned, and the explanation entirely up to the reader's judgment. :)

core/cap-0034.md Outdated

## Abstract

We change the (deterministic) mechanism by which the ballot protocol computes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be better if you broke this down into some simpler sentences, eg. "The protocol currently produces a candidate value for the ballot protocol by .... We propose producing a candidate value for the ballot protocol by ..." Just an example, but I think it would make this a lot easier to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, wow, that sentence did get rather long. I broke it up.

core/cap-0034.md Outdated
`SIGNED` `StellarValue`s in some places where `BASIC` ones are currently used,
but the XDR itself does not change.

To date, the ballot protocol has generated a value to externalize by combining
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just remove this paragraph entirely from this section. This is the specification: I want to know exactly how it will work but everything else is irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

core/cap-0034.md Outdated
value as the best transaction set. (It proposes to preserve the selection
heuristic for "best transaction set" unchanged.)

Because the closetime and transaction set in the externalized value would
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, there isn't really a need to justify why you are doing things here. Just tell the reader in simple terms what it is. The goal is to make this section as precise and concise as possible so that it is easy to turn it directly into an implementation. Justifications you can move to the design rationale section.

It is probably worth revisiting this entire section with this goal in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've tried to trim the specification down to just specifying. The justifications were, I think, just redundant with the ones already in the "Design Rationale".

not have _relied_ on that inconsistency, as it requires a race to occur,
and one which is probably very rare.

## Security Concerns
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is about the security concerns for this proposal. If there were security concerns that caused us not to choose other proposals, then that should be in the design rationale.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay -- it was in the "Design Rationale" already, so I simply removed it as redundant from the "Security Concerns" section (and referenced the "Design Rationale" section there).

@jonjove jonjove marked this pull request as ready for review July 9, 2020 14:29
First draft of CAP-0034, intended to address
stellar-protocol issue stellar#622, by changing the semantics of
the selection of the closetime when combining candidates in
the SCP nomination protocol.

The current nomination protocol produces an unsigned
("basic") form of union of the candidates which uses the
maximum closetime of all candidates as the closetime to
nominate.

The protocol proposed by this CAP would produce a signed
StellarValue, whose transaction set and upgrades are chosen
in the same way as in the current protocol, but whose
closetime is precisely the one in the same StellarValue as
the best transaction set.

(The change to closetime semantics is sufficient by itself
to address stellar-protocol issue stellar#622; the change to make
the nominated StellarValue signed is an opportunity that we
take which the closetime semantics change provides.  The
signature is on the <txSet, closetime> pair, so by taking
the exact closetime from the same StellarValue as the chosen
transaction set, we allow ourselves to use the signature
from the node that proposed that StellarValue.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants