Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Book: Update SPV section to reflect new account state query mechanism #5399

Merged
t-nelson merged 4 commits intosolana-labs:masterfrom
t-nelson:book-spv_acct_state_verify
Jan 10, 2020
Merged

Book: Update SPV section to reflect new account state query mechanism #5399
t-nelson merged 4 commits intosolana-labs:masterfrom
t-nelson:book-spv_acct_state_verify

Conversation

@t-nelson
Copy link
Copy Markdown
Contributor

@t-nelson t-nelson commented Aug 1, 2019

As per Discord convo with @aeyakovenko and @rob-solana

@t-nelson t-nelson requested a review from aeyakovenko August 1, 2019 22:48
such as a credit of 0 Lamports.
An account's state (balance or other data) can be verified by submitting a
transaction with a ___TBD___ Instruction to the cluster. This instruction contains
the slot boundary of interest and the expected state hash at that boundary. The
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can’t enforce the slot boundary for this query. I think it’s basically is the query true when it’s executed by the cluster.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hrm. That's a pretty narrow window to hit on a high throughput account, basically the next slot and 🤞, right?

Copy link
Copy Markdown
Contributor

@rob-solana rob-solana Aug 2, 2019

Choose a reason for hiding this comment

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

to be able to do "slot of interest": would it be terribly expensive to hang onto the account hash vector at the end of every slot for 256 slots? worst case is 256*2.5*sizeof(Hash)*TPS, similar to StatusCache

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@t-nelson why would the slot boundary provide any safety? The state can change immediately on the next slot. It’s up to the contract to have some meaningful terminal state, like “game over”

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rob-solana by "hash vector" you mean the "inclusion proof" or "merkle path"? They'd be log(N) and mergable per-slot, so that's certainly something to explore

@aeyakovenko there's no safety at the boundary. I was getting at the race between receiving the "Don't squash this slot" instruction and the slot being squashed, though after review Carl's snapshot work, I realize we have until the bank in question falls out of the locktower. This will require us scanning all bank forks and taking action on an unfinalized transaction, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@t-nelson we can take action on it after that bank is rooted, and provide the result way later, maybe 1024 slots after or something long enough, or at an epoch boundary.

@aeyakovenko
Copy link
Copy Markdown
Member

Cool! This is missing the checkpoint proof generation.

@t-nelson
Copy link
Copy Markdown
Contributor Author

t-nelson commented Aug 2, 2019

Oof. I got to thinking about whether there was potential for a cheap resource exhaustion attack here and forgot to write it up 😆

@aeyakovenko
Copy link
Copy Markdown
Member

aeyakovenko commented Aug 5, 2019

@t-nelson so I think this would work on Ethereum.
vote for BankHash N, links to BankHash N -1

Since the BankHash's are connected, the SPV check on ethereum can skip PoH.

@pgarg66 I think this would replace the "Ancestor Verification" section in:
https://solana-labs.github.io/book/vote-signing-to-implement.html#ancestor-verification

@garious
Copy link
Copy Markdown
Contributor

garious commented Aug 12, 2019

@t-nelson, what's this PR blocked by?

@t-nelson t-nelson force-pushed the book-spv_acct_state_verify branch from 7edb070 to afd7dee Compare August 13, 2019 17:19
@t-nelson
Copy link
Copy Markdown
Contributor Author

@aeyakovenko Sorry, this fell off my radar with the colo setup stuff. Last two commits address the issues I think. LMK what you think

@t-nelson t-nelson requested a review from aeyakovenko August 19, 2019 16:23
@t-nelson
Copy link
Copy Markdown
Contributor Author

@rob-solana, since @aeyakovenko is travelling, would you mind giving this a once over to see if it jives with your understanding of the proposal?

@rob-solana
Copy link
Copy Markdown
Contributor

@rob-solana, since @aeyakovenko is travelling, would you mind giving this a once over to see if it jives with your understanding of the proposal?

I think @sakridge has better information about the current plan.

@sakridge
Copy link
Copy Markdown
Contributor

So we are considering making the accounts hash value as:

expand_fn(hash(account0)) ^ expand_fn(hash(account1)) ^ ...

where expand_fn(hash) -> [u8; 440] is generating 440 bytes of random data from a crypto RNG with the account hash as the seed value. This allows one to add/remove accounts from the state value without re-computing the entire hash value.

@sakridge
Copy link
Copy Markdown
Contributor

440 bytes comes from this paper, avoiding xor collision with 0 (or thus any other given bit pattern):
https://link.springer.com/content/pdf/10.1007%2F3-540-45708-9_19.pdf

O(k · 2^(n/(1+lg(k)))
k=2^40 accounts
n=440
2^(40) * 2^(448 * 8 / 41) ~= O(2^(128))

@t-nelson
Copy link
Copy Markdown
Contributor Author

@sakridge OK thanks! Funny, I was actually typing up some clarifications that your second reply addressed 🙂

I get that it's cheaper to calculate and add/remove entries from this construction, but I don't see how we can build an inclusion proof? Or is that a tangential requirement?

@sakridge
Copy link
Copy Markdown
Contributor

@t-nelson well, I was mainly after the performance side of it compared to what we are doing now or potentially computing a merkle tree of the accounts. @aeyakovenko had some idea that it would reduce the need to compute a merkle tree with old account states but I can't say I fully grok how that works yet.

@t-nelson
Copy link
Copy Markdown
Contributor Author

@sakridge Gotcha! I think you and I are on the same page. We'll have to await input from the big guy 🙃

@rob-solana
Copy link
Copy Markdown
Contributor

So we are considering making the accounts hash value as:

expand_fn(hash(account0)) ^ expand_fn(hash(account1)) ^ ...

where expand_fn(hash) -> [u8; 440] is generating 440 bytes of random data from a crypto RNG with the account hash as the seed value. This allows one to add/remove accounts from the state value without re-computing the entire hash value.

oh man, this is gonna get messy... have a PR yet?

@sakridge
Copy link
Copy Markdown
Contributor

@rob-solana #5573 this is the PR. Yea, it's not super clean, but not the worst. Not sure how it combines with what changes you are planning.

such as a credit of 0 Lamports.
A complication of this method is avoiding bank accounts squashing discarding
information required to verify blocks referenced by an inclusion proof. A solution
is to instead snapshot/checkpoint these banks by submitting a ___TBDb_Instruction___,
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is no longer necessary. The bank hash is derived from the snapshot image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But we may want the bank hash to be the hash of the block transaction merkle root, hash of the snapshot image and the previous bank hash

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

What about the accounts XOR? I think I missed some convo on that somewhere (didn't seem to be more in discord/#consensus, anyway). Do we not need inclusion proofs on the account state anymore?

Oh, and if you're on vacation, feel free to tell me to shove off 🙂

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we don’t have an account inclusion proof. So you still need a tx. Shove off :)

with the Previous Bank-Merkle, and the Block-Merkle.
An account's state (balance or other data) can be verified by submitting a
transaction with a ___TBDa_Instruction___ to the cluster. The client can then use
a [Transaction Inclusion Proof](#transaction-inclusion-proof) to verify whether
Copy link
Copy Markdown
Contributor

@carllin carllin Aug 26, 2019

Choose a reason for hiding this comment

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

Is the purpose of the account state verification to prove that an account has some expected state, at some given point in the past (some block)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ah ok, I see, the Accounts-Hash only includes accounts that were modified during a slot, which is why a transaction inclusion proof of the transaction that caused the modification is necessary. Thanks!

or
* Transaction -> Entry-Merkle -> Block-Merkle -> Bank-Merkle

* Transaction -> Entry-Merkle -> Block-Merkle -> Bank-Hash
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we are generating the SPV of a transaction in a block A, for the vector of validator vote entries mentioned below, do we include all future votes for each validator up until MAX_LOCKOUT? If not, what is the locktower depth that we are interested in for confirmation?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The confirmation depth should be configurable (with a safe min and default) and would be dependent on the economic value of the transaction. $10 TX, low confirmation, $10M TX, high confirmation.

Note that this is a little WIP ATM as typically an SPV client would be validating block headers on its own and not have to reveal this information to the full node providing the TX proof. On Solana, this would require them to be capable of keeping up with and validating PoH, which is an atypically high standard for such a client.

@stale
Copy link
Copy Markdown

stale Bot commented Sep 25, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 25, 2019
@garious
Copy link
Copy Markdown
Contributor

garious commented Sep 26, 2019

I haven't been following this one. Is there a subset of stuff you guys agree on that we can get merged?

@stale stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 26, 2019
@t-nelson
Copy link
Copy Markdown
Contributor Author

This needs updated for #5885. I'll try to sync it up today.

@garious
Copy link
Copy Markdown
Contributor

garious commented Oct 15, 2019

@t-nelson, can you wrap this up?

@t-nelson
Copy link
Copy Markdown
Contributor Author

Can do

@t-nelson t-nelson force-pushed the book-spv_acct_state_verify branch from afd7dee to f5b4350 Compare October 23, 2019 22:36
@stale
Copy link
Copy Markdown

stale Bot commented Nov 22, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale
Copy link
Copy Markdown

stale Bot commented Nov 29, 2019

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale Bot closed this Nov 29, 2019
@garious garious reopened this Nov 29, 2019
@stale stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 29, 2019
@stale
Copy link
Copy Markdown

stale Bot commented Dec 17, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 17, 2019
@t-nelson t-nelson removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 17, 2019
@stale
Copy link
Copy Markdown

stale Bot commented Dec 24, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 24, 2019
@stale
Copy link
Copy Markdown

stale Bot commented Dec 31, 2019

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale Bot closed this Dec 31, 2019
@t-nelson t-nelson removed the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 31, 2019
@t-nelson t-nelson reopened this Dec 31, 2019
@stale
Copy link
Copy Markdown

stale Bot commented Jan 7, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale Bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 7, 2020
@t-nelson t-nelson force-pushed the book-spv_acct_state_verify branch from f5b4350 to 828dda3 Compare January 9, 2020 22:51
@stale stale Bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Jan 9, 2020
@t-nelson t-nelson force-pushed the book-spv_acct_state_verify branch from 828dda3 to 5a7523b Compare January 9, 2020 22:55
@garious
Copy link
Copy Markdown
Contributor

garious commented Jan 10, 2020

Can you format to 80 char lines?

@t-nelson
Copy link
Copy Markdown
Contributor Author

Absolutely

@garious
Copy link
Copy Markdown
Contributor

garious commented Jan 10, 2020

@t-nelson, waiting on action from anyone here?

@t-nelson t-nelson merged commit 2356b25 into solana-labs:master Jan 10, 2020
@t-nelson t-nelson deleted the book-spv_acct_state_verify branch January 10, 2020 17:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants