Add isBlockhashValid method to web3.js #25888

zhe-t · 2022-06-09T22:08:32Z

Problem

Attempting to resolve solana-labs/solana-web3.js#1113 - Add isBlockhashValid support

Summary of Changes

Added isBlockhashValid method to web3.js sdk

Fixes #

steveluscher

Thanks, @zhe-t!

Now all this PR needs is some tests. Use the ‘get block time’ test in connection.test.ts as a starting point. Run both yarn test and yarn:test-live-with-test-validator. You can turn on only the test you're working on by adding .only to the end of it or describe.

Write tests that cover:

the RPC returning a value when you supply a blockhash
the RPC returning a value when you supply a blockhash and a min slot
(LIVE mode only) getting the recent blockhash then asking if it's valid (expect to be true)
(LIVE mode only) getting the recent blockhash and slot then asking if it's valid with a min slot one slot higher (expect to be false)
(LIVE mode only) supplying a junk blockhash then asking if it's valid (expect to be false)

web3.js/src/connection.ts

…son rpc result

Pull request has been modified.

zhe-t · 2022-06-10T11:32:25Z

Thanks @steveluscher - working on tests now.

zhe-t · 2022-06-10T12:34:49Z

@steveluscher could you help me with the

(LIVE mode only) getting the recent blockhash and slot then asking if it's valid with a min slot one slot higher (expect to be false)

test please?

The test is called is blockhash valid - blockhash only and mint slot (live) in ca50893#diff-0ff00ad7672d836c05bc2a1180365eb6a757b5e06c80edc7e9b8a7456b9a52d6R2991.

I must have over looked something. The recent slot is always 0 when using yarn:test-live-with-test-validator.

steveluscher · 2022-06-11T03:15:22Z

The recent slot is always 0 when using yarn:test-live-with-test-validator.

For sure! In cases where it's the first test you run, this will be the case.

Just loop, waiting for a minimum of slots to pass.

solana/web3.js/test/connection.test.ts

Lines 2969 to 2971 in ed5fcde

    
           while ((await connection.getSlot()) <= 1) { 
        
             continue; 
        
           }

…than latest finalised slot

zhe-t · 2022-06-13T12:35:31Z

Tests added

the RPC returning a value when you supply a blockhash
the RPC returning a value when you supply a blockhash and a min slot
(LIVE mode only) getting the recent blockhash then asking if it's valid (expect to be true)
(LIVE mode only) getting the recent blockhash and slot then asking if it's valid with a min slot one slot higher (expect to be false)
(LIVE mode only) supplying a junk blockhash then asking if it's valid (expect to be false)

I've tried using the slot returned by getLatestBlockhashAndContext and using getSlot (which are both now non-0) but the method returns a true value when using this slot value+1. Do you have an idea of what I have overlooked? @steveluscher

zhe-t · 2022-06-18T21:49:26Z

cc @steveluscher

steveluscher · 2022-06-18T22:10:42Z

Sorry for the delay here!

I don't really know, but here are some thoughts:

One of the values that you get back from getLatestBlockhash is one called lastValidBlockheight. What happens if you set minContextSlot to lastValidBlockheight + 1?
Do you have to wait for the lastValidBlockheight to pass before making your assertion?

I don't really know what the effect of minContextSlot is here, ~~but what I do know is that blockhashes are only valid up to a certain slot, and that slot is given by lastValidBlockheight~~.

jstarry · 2022-06-20T11:06:26Z

What happens if you set minContextSlot to lastValidBlockheight + 1?

Block height and slot are two totally different things fyi

jstarry

Great to see minContextSlot being used here :)

web3.js/src/connection.ts

jstarry · 2022-06-20T11:11:04Z

web3.js/src/connection.ts

+    const unsafeRes = await this._rpcRequest('isBlockhashValid', args);
+    const res = create(unsafeRes, jsonRpcResultAndContext(boolean()));
+    if ('error' in res) {
+      throw new Error(


I think we should have special handling for when the min context slot isn't met. It can be detected from its error code:

pub const JSON_RPC_SERVER_ERROR_MIN_CONTEXT_SLOT_NOT_REACHED: i64 = -32016;

We could retry a few times (configurable in IsBlockhashValidConfig). What do you think? I wouldn't mind this change coming later in a separate PR too. So maybe we just ensure that the thrown error object includes the error code as an integer

Neat. Two questions come to mind:

Should we check for this error code in res and retry a configurable amount of times before bailing?

Should we return the error code be propagated in the response object of IsBlockhashValid?

yup, that's what I had in mind

no, it should be abstracted away

Shall I include this in the current PR? @jstarry @steveluscher

steveluscher · 2022-06-20T16:08:27Z

Block height and slot are two totally different things fyi

Oof. Of course. Sorry for the misdirection @zhe-t.

steveluscher · 2022-06-20T16:10:03Z

web3.js/test/connection.test.ts

+      it('is blockhash valid - blockhash only and mint slot (live)', async () => {
+        const blockhash = await connection.getLatestBlockhashAndContext('finalized');
+
+        const isValid = await connection.isBlockhashValid(
+          blockhash.value.blockhash,
+          {
+            commitment: 'finalized',
+            minContextSlot: blockhash.context.slot+1
+          });
+        expect(isValid.value).to.be.false;
+      });


Do you have any advice on how to write a test that both exercises minContextSlot and fails, @jstarry? I'm missing the plot here.

How about setting it to the max safe integer?

Setting to MAX_SAFE_INTEGER gives the same result 🤔 validation result is returning as true

It's because your code isn't actually sending minContextSlot in the rpc request

I love it when writing tests pays off right away.

web3.js/src/connection.ts

steveluscher

Marking as request changes -> back to your queue.

Pull request has been modified.

zhe-t · 2022-07-04T13:05:30Z

Updated the PR to resolve the config params not being parsed/passed correctly and now all tests are passing 🎉 thanks @jstarry @steveluscher for the guidance. Ready to review if we do not want to include the special handling for when the min context slot is not met.

steveluscher

I pushed up a few commits to finish this up, but could you take a look at the tests @zhe-t? Write one set of tests that works in both live mode and mock mode. The key is that mockRpcResponse is a no-op in LIVE mode. Write the tests so that they would pass whether mockRpcResponse takes effect or not.

steveluscher · 2022-07-06T20:35:28Z

web3.js/src/connection.ts

+    blockhash: string,
+    config?: IsBlockhashValidConfig
+  ): Promise<RpcResponseAndContext<boolean>> {
+    const extra: Pick<IsBlockhashValidConfig, 'minContextSlot'> = {};


In general, you want to take an Omit approach rather than a Pick approach.

const extra = Omit<IsBlockhashValidConfig, 'commitment'> = {};

steveluscher · 2022-07-06T20:36:09Z

web3.js/src/connection.ts

+    let commitment;
+    if (config) {
+      if (config.minContextSlot) {
+        extra.minContextSlot = config.minContextSlot;
+      }
+
+      if (config.commitment) {
+        commitment = config.commitment;
+      } else {
+        commitment = this._commitment || 'finalized';
+      }
+    } else {
+      commitment = this._commitment || 'finalized';
+    }


Since you wrote this PR, we created a helper for this!

const {commitment, config} = extractCommitmentFromConfig(...);

steveluscher · 2022-07-06T20:37:15Z

web3.js/src/connection.ts

+    const unsafeRes = await this._rpcRequest('isBlockhashValid', args);
+    const res = create(unsafeRes, jsonRpcResultAndContext(boolean()));
+    if ('error' in res) {
+      console.log(res.error.message);


steveluscher · 2022-07-06T20:38:28Z

web3.js/src/connection.ts

+    const res = create(unsafeRes, jsonRpcResultAndContext(boolean()));
+    if ('error' in res) {
+      console.log(res.error.message);
+      throw new Error(


Since you made this PR, we introduced SolanaJSONRPCError.

steveluscher · 2022-07-06T20:43:15Z

web3.js/test/connection.test.ts

+      await mockRpcResponse({
+        method: 'isBlockhashValid',
+        params: [blockhash],
+        value: true,
+        withContext: true
+      });


The mockRpcResponse helper actually skips when you're in live mode, so that you only have to write the tests once.

codecov · 2022-07-06T21:14:25Z

Codecov Report

Merging #25888 (ff51656) into master (627d91f) will decrease coverage by 0.0%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master   solana-labs/solana#25888     +/-   ##
=========================================
- Coverage    76.0%    76.0%   -0.1%     
=========================================
  Files          40       40             
  Lines        2370     2377      +7     
  Branches      343      344      +1     
=========================================
+ Hits         1803     1808      +5     
- Misses        449      450      +1     
- Partials      118      119      +1

stale · 2022-08-13T09:01:50Z

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.

Add isBlockhashValid method to web3.js

b6332f9

mergify bot added the community Community contribution label Jun 9, 2022

mergify bot requested a review from a team June 9, 2022 22:08

steveluscher previously requested changes Jun 10, 2022

View reviewed changes

web3.js/src/connection.ts Outdated Show resolved Hide resolved

web3.js/src/connection.ts Outdated Show resolved Hide resolved

web3.js/src/connection.ts Outdated Show resolved Hide resolved

steveluscher mentioned this pull request Jun 10, 2022

web3.js: Add isBlockhashValid support solana-labs/solana-web3.js#1113

Closed

zhe-t marked this pull request as draft June 10, 2022 11:13

Refactor isBlockhashValid to use configOrCommitment, removed unused j…

1c08c37

…son rpc result

mergify bot requested a review from a team June 10, 2022 11:31

zhe-t added 2 commits June 10, 2022 13:31

Fix response type for isBlockhashValid method

6ef0a2a

Add isBlockhashValid tests

ca50893

Add junk blockhash test

c440a9e

zhe-t marked this pull request as ready for review June 10, 2022 12:49

Merge branch 'solana-labs:master' into master

b04ff28

zhe-t added 2 commits June 13, 2022 13:31

Update isBlockhashValid test (live) with min context slot one higher …

3333af4

…than latest finalised slot

Merge branch 'master' of https://github.com/zhe-t/solana

88387b9

jstarry reviewed Jun 20, 2022

View reviewed changes

steveluscher reviewed Jun 20, 2022

View reviewed changes

zhe-t added 2 commits June 20, 2022 18:30

Capitalise IsBlockhashValidConfig, remove commitment param type

762f277

Update mix context slot to max safe integer

87d2bb6

jstarry reviewed Jun 23, 2022

View reviewed changes

web3.js/src/connection.ts Outdated Show resolved Hide resolved

steveluscher previously requested changes Jun 27, 2022

View reviewed changes

Fix incorrectly parsed config in IsBlockhashValid method

1ec6a95

Merge branch 'solana-labs:master' into master

7040f0e

zhe-t requested review from steveluscher and jstarry July 4, 2022 23:15

steveluscher added 4 commits July 6, 2022 13:56

chore: remove console log

07e97a0

chore: format code

88f1852

chore: throw new error type

b2d82bf

chore: use helper to tease apart commitment and config

ff51656

steveluscher requested changes Jul 6, 2022

View reviewed changes

stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 13, 2022

steveluscher removed the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 13, 2022

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Dec 29, 2022

github-actions bot closed this Jan 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add isBlockhashValid method to web3.js #25888

Add isBlockhashValid method to web3.js #25888

zhe-t commented Jun 9, 2022

steveluscher left a comment

zhe-t commented Jun 10, 2022

zhe-t commented Jun 10, 2022 •

edited

Loading

steveluscher commented Jun 11, 2022

zhe-t commented Jun 13, 2022 •

edited

Loading

zhe-t commented Jun 18, 2022

steveluscher commented Jun 18, 2022 •

edited

Loading

jstarry commented Jun 20, 2022

jstarry left a comment

jstarry Jun 20, 2022

zhe-t Jun 20, 2022

jstarry Jun 23, 2022

zhe-t Jul 4, 2022 •

edited

Loading

steveluscher commented Jun 20, 2022

steveluscher Jun 20, 2022

jstarry Jun 20, 2022

zhe-t Jun 23, 2022

jstarry Jun 23, 2022

steveluscher Jun 23, 2022

steveluscher left a comment

zhe-t commented Jul 4, 2022

steveluscher left a comment

steveluscher Jul 6, 2022

steveluscher Jul 6, 2022

steveluscher Jul 6, 2022

steveluscher Jul 6, 2022

steveluscher Jul 6, 2022

codecov bot commented Jul 6, 2022

stale bot commented Aug 13, 2022

Add isBlockhashValid method to web3.js #25888

Add isBlockhashValid method to web3.js #25888

Conversation

zhe-t commented Jun 9, 2022

Problem

Summary of Changes

steveluscher left a comment

Choose a reason for hiding this comment

zhe-t commented Jun 10, 2022

zhe-t commented Jun 10, 2022 • edited Loading

steveluscher commented Jun 11, 2022

zhe-t commented Jun 13, 2022 • edited Loading

zhe-t commented Jun 18, 2022

steveluscher commented Jun 18, 2022 • edited Loading

jstarry commented Jun 20, 2022

jstarry 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

zhe-t Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

steveluscher commented Jun 20, 2022

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

steveluscher left a comment

Choose a reason for hiding this comment

zhe-t commented Jul 4, 2022

steveluscher 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

codecov bot commented Jul 6, 2022

Codecov Report

stale bot commented Aug 13, 2022

zhe-t commented Jun 10, 2022 •

edited

Loading

zhe-t commented Jun 13, 2022 •

edited

Loading

steveluscher commented Jun 18, 2022 •

edited

Loading

zhe-t Jul 4, 2022 •

edited

Loading