SDK: Add sysvar to expose recent block hashes to programs by t-nelson · Pull Request #6663 · solana-labs/solana

t-nelson · 2019-10-31T20:07:34Z

Problem

Recent blockhashes aren't available to programs

Summary of Changes

Expose a subset of the bank's blockhash_queue via sysvar

garious · 2019-10-31T20:38:28Z

All uses of BlockHash and block_hash need to be Blockhash and blockhash respectively. In Solana, blockhash is one word, not two. Also, can you add that term to the book? I see the code is nicely consistent, but that term never made it to the terminology page.

garious · 2019-10-31T20:41:58Z

btw, love seeing the unit tests at multiple levels. Great PR!

codecov · 2019-10-31T20:43:32Z

Codecov Report

Merging #6663 into master will increase coverage by 15.2%.
The diff coverage is 93.6%.

@@            Coverage Diff            @@
##           master   #6663      +/-   ##
=========================================
+ Coverage    63.3%   78.6%   +15.2%     
=========================================
  Files         220     213       -7     
  Lines       51718   41304   -10414     
=========================================
- Hits        32772   32483     -289     
+ Misses      18946    8821   -10125

t-nelson · 2019-10-31T20:49:32Z

I'll add terminology in a separate PR

Pull request has been modified.

garious · 2019-10-31T20:56:27Z

One concern, it looks like this adds two mallocs per slot, and it could be zero. What if we get the previous account and then just overwrite its blockhashes?

t-nelson · 2019-10-31T22:23:41Z

@garious like this?

486f017#diff-a7549f152920d85fb44e6a784b8e5e1fR547-R552

garious · 2019-10-31T22:30:08Z

@t-nelson, that gets rid of one malloc. To get rid of the other, you should return an impl Iterator instead of a Vec.

garious · 2019-10-31T22:32:04Z

+    Account::new(lamports, RecentBlockhashes::size_of(), &sysvar::id())
+}
+
+pub fn update_account(account: &mut Account, recent_blockhashes: Vec<Hash>) -> Option<()> {


This should accept an IntoIterator instead of Vec.

garious · 2019-10-31T22:32:33Z

        None
    }
+
+    pub fn get_recent_blockhashes(&self, count: usize) -> Vec<Hash> {


This should return an impl Iterator instead of a Vec

On second thought. Hold off. That recent_blockhashes doesn't look to be the Dequeue it used to be. Surprised to see the need to sort there.

So why does this have a count parameter? It's not used by anything other than the test. And if it wasn't there, you could return an unordered impl Iterator<Item = &Hash> with effectively zero cost.

count was an attempt to shortcircuit the last iteration and avoid another to truncate in the caller. Returning an impl Iterator<Item=&Hash though, I can move all of that logic to the caller (and avoid the hash copies)

t-nelson · 2019-10-31T22:49:31Z

Ah cool! Gimme a few

garious · 2019-10-31T22:49:54Z

+        let id = sysvar::recent_blockhashes::id();
+        let mut account = self
+            .get_account(&id)
+            .or_else(|| Some(sysvar::recent_blockhashes::create_account(1)))


unwrap_or_else

garious · 2019-10-31T22:55:45Z

+}
+
+pub fn update_account(account: &mut Account, recent_blockhashes: Vec<Hash>) -> Option<()> {
+    let recent_blockhashes = RecentBlockhashes(recent_blockhashes);


@rob-solana, ideally, this would be a zero-copy from_account(account) and then replace the contents of RecentBlockhashes.0 with that from the recent_blockhashes input. And if so, then there's be no need for that to_account() afterward.

I wanna see a helper library for zero copy serde

pushing a brand new recent_blockhashes into the accounts_db is faster than reading, updating, storing an update....

pushing a brand new recent_blockhashes into the accounts_db is faster than reading, updating, storing an update....

Unexpected... So 486f017 is counterproductive?

Probably. I think we need prioritize clear, concise tests and a micro-benchmark, as all this code will change once somebody figures our a good way to do zero-copy deserialization.

I went ahead and reverted the fetch+update. It costs ~1.3us

garious · 2019-10-31T22:59:51Z

@t-nelson, since this function gets called once every 400ms, I think this seemingly innocent feature needs a microbenchmark. Between the serialization, the vec mallocs, and hash copies, there's a lot that can be optimized here.

garious · 2019-11-01T16:04:57Z

The PR problem description doesn't describe a problem.

t-nelson · 2019-11-02T20:00:44Z

This takes ~12.3us. With the account fetch+update ~13.6us. Without iterators ~18.5us.

This reverts commit a8f8c3b.

This reverts commit 486f017.

garious

Nice, thanks for the deep dive!

ryoqun · 2020-01-22T23:08:32Z

+    I: IntoIterator<Item = (u64, &'a Hash)>,
+{
+    let sorted = BinaryHeap::from_iter(recent_blockhash_iter);
+    let recent_blockhash_iter = sorted.into_iter().take(MAX_ENTRIES).map(|(_, hash)| hash);


This is a bug which will be fixed by #7918.

SDK: Add sysvar to expose recent block hashes to programs

38a3d08

t-nelson requested a review from rob-solana October 31, 2019 20:07

rob-solana previously approved these changes Oct 31, 2019

View reviewed changes

Blockhashes is one word

054e1fa

t-nelson added 2 commits October 31, 2019 16:21

Missed one

5211e46

Avoid allocs on update

486f017

garious reviewed Oct 31, 2019

View reviewed changes

t-nelson added 2 commits November 2, 2019 13:16

unwrap_or_else

a8f8c3b

Use iterators

e83a321

t-nelson added 3 commits November 2, 2019 14:02

Add microbench

42c5a5f

Revert "unwrap_or_else"

1c76a87

This reverts commit a8f8c3b.

Revert "Avoid allocs on update"

14b26b1

This reverts commit 486f017.

t-nelson force-pushed the recent_block_hashes_sysvar branch from 5b3ebe4 to 14b26b1 Compare November 2, 2019 20:02

garious approved these changes Nov 2, 2019

View reviewed changes

t-nelson merged commit 5416c11 into solana-labs:master Nov 4, 2019

t-nelson deleted the recent_block_hashes_sysvar branch November 4, 2019 17:51

t-nelson mentioned this pull request Nov 4, 2019

Book: Add blockhash to terminology #6711

Merged

ryoqun reviewed Jan 22, 2020

View reviewed changes

ryoqun mentioned this pull request Jan 22, 2020

Avoid unsorted recent_blockhashes for determinism #7918

Merged

Conversation

t-nelson commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

garious commented Oct 31, 2019

Uh oh!

garious commented Oct 31, 2019

Uh oh!

codecov Bot commented Oct 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

t-nelson commented Oct 31, 2019

Uh oh!

garious commented Oct 31, 2019

Uh oh!

t-nelson commented Oct 31, 2019

Uh oh!

garious commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t-nelson commented Oct 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garious commented Oct 31, 2019

Uh oh!

garious commented Nov 1, 2019

Uh oh!

t-nelson commented Nov 2, 2019

Uh oh!

garious left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

t-nelson commented Oct 31, 2019 •

edited

Loading

codecov Bot commented Oct 31, 2019 •

edited

Loading