rpc: Error gSFA if before/until not found

[NicolasPennie]

Problem

getSignaturesForAddress would return an empty vector if the before parameter did not exist in either blockstore or bigtable. It would return all results in blockstore if the until parameter did not exist, even if no data was found in bigtable.

These are silent errors. The client will not be aware that their query parameters are no longer respected and the returned data is (potentially) incorrect.

Summary of Changes

The below table desribes the behavior for each paramater and any changes:

Param	Blockstore	BigTable	Outcome	Changed ?	Notes
before	not found	not found	error	Yes (previously Ok([])
before	not found	found	success	No
before	found	not found	success	No	Bigtable continues fetch from latest Blockstore sig
before	found	found	success	No	Bigtable continues fetch from latest Blockstore sig
until	not found	not found	error	Yes (previously Ok([])
until	not found	found	success	No
until	found	not found	success	No (see note)	Skip Bigtable fetch is not needed since Blockstore found until
until	found	found	success	No (see note)	Skip Bigtable fetch is not needed since Blockstore found until

And then if BigTable is not initialized, all requests will error if before or until is not found because we have no way to guarantee correctness.

[mergify]

If this PR represents a change to the public RPC API:

1. Make sure it includes a complementary update to rpc-client/ (example)
2. Open a follow-up PR to update the JavaScript client @solana/kit (example)

Thank you for keeping the RPC clients in sync with the server API @NicolasPennie.

[joncinque]

Thanks for your PR! I mostly have small comments, and I'd like to refine the table to add what's been changed in this PR:

Param	Blockstore	BigTable	Outcome	This PR
before	X	X	error	Changed, used to be []
before	X	✔️	success	No change
before	✔️	?	impossible because fetch from the last BS signature	No Change
until	X	X	error	Changed, used to be []
until	X	✔️	success	No change
until	✔️	?	doesn't matter because found	Changed (with my comment), but no impact on result

Does this table make more sense? I'm mainly clarifying the "impossible" situations, since I believe those are currently possible.

Edit: sorry, the table looks so much nicer in preview for some reason, trying to make it look nice again

[joncinque]

When using getTransaction with a signature that doesn't exist, we just return null rather than an error.

An error type makes more sense to me than returning null in this situation, so we'll end up doing something different here. Either way, we'll need to add the new error to kit as well, as the automated message mentions.

@steveluscher do you have a preference here?

[steveluscher]

I'm fine with this being a JSON RPC error, but I wonder if it would be useful either:

• Adding whether the not-found signature was in the before or until position
• Making this two errors, one for before and one for until

It feels sort of bad making the developer guess what filter position the not-found signature was in, but I also don't know if it matters.

[NicolasPennie]

Thanks for your PR! I mostly have small comments, and I'd like to refine the table to add what's been changed in this PR:

Param Blockstore BigTable Outcome This PR
before X X error Changed, used to be []
before X ✔️ success No change
before ✔️ ? impossible because fetch from the last BS signature No Change
until X X error Changed, used to be []
until X ✔️ success No change
until ✔️ ? doesn't matter because found Changed (with my comment), but no impact on result
Does this table make more sense? I'm mainly clarifying the "impossible" situations, since I believe those are currently possible.

Edit: sorry, the table looks so much nicer in preview for some reason, trying to make it look nice again

correct!

[joncinque]

@steviez can you take a look at the change? The only difference from @NicolasPennie 's work was avoiding going into big-table if until was found, and fixing clippy warnings, the rest should be straightforward

[codecov-commenter]

Codecov Report

❌ Patch coverage is 19.04762% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.3%. Comparing base (d55741a) to head (f840452).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##           master    #7937     +/-   ##
=========================================
- Coverage    83.3%    83.3%   -0.1%     
=========================================
  Files         845      845             
  Lines      319955   319985     +30     
=========================================
- Hits       266772   266769      -3     
- Misses      53183    53216     +33     

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[steviez]

Apologies on the delay, will try to get to this one this week

[steviez]

Hi @NicolasPennie - Apologies for the delays here. I intend to actually get around to reviewing this in the next couple days. In the meantime, would you mind rebasing on the tip of master ? I don't see any merge conflicts but just better safe than sorry (ie CI)

[joncinque]

I've taken over this PR for Nick, so I've pushed up the rebase

[mergify]

Backports to the beta branch are to be avoided unless absolutely necessary for fixing bugs, security issues, and perf regressions. Changes intended for backport should be structured such that a minimum effective diff can be committed separately from any refactoring, plumbing, cleanup, etc that are not strictly necessary to achieve the goal. Any of the latter should go only into master and ride the normal stabilization schedule. Exceptions include CI/metrics changes, CLI improvements and documentation updates on a case by case basis.

[steviez]

LGTM - I agree that returning Ok(vec![]) is incorrect and returning an Err is better behavior. I'd arguably the old behavior is buggy (or at least not intuitive) so I also think reasonable to pull back into v4.0 (small change)

I had one nit - your (Jon) call on whether you want to address or not. Happy to restamp if you like the change, fine if you merge the PR as-is too

[steviez]

@joncinque - One other note, I updated the table @NicolasPennie created in the PR description. Would you mind double checking that and making sure that what I put in is accurate / makes sense ?

[joncinque]

The table looks good to me!