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

RPC: use finalized as default pubsub commitment level #16659

Merged
merged 3 commits into from
Apr 20, 2021

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Apr 20, 2021

Problem

The default commitment for pubsub and http requests are different which can cause consistency issues in downstream clients.

Summary of Changes

Change the default commitment of pubsub subscriptions from confirmed to finalized

Fixes #16596

@mvines
Copy link
Contributor

mvines commented Apr 20, 2021

Minor wrinkle here I guess is that we explicitly document the default as "confirmed" :-/
https://github.com/solana-labs/solana/search?q=%22For+subscriptions%2C+if+commitment+is+unspecified%2C+the+default+value+is%22

I think it's fair to call this a bug, but it should be called out as a breaking change in the release notes. Does web3.js have anything to say about the default in comments anywhere?

@jstarry
Copy link
Contributor Author

jstarry commented Apr 20, 2021

Minor wrinkle here I guess is that we explicitly document the default as "confirmed" :-/

Dang, will fix

I think it's fair to call this a bug, but it should be called out as a breaking change in the release notes.

Agreed on both points

Does web3.js have anything to say about the default in comments anywhere?

Nope, it even specifies that the "max" is the default here: https://solana-labs.github.io/solana-web3.js/modules.html#sendandconfirmtransaction

mvines
mvines previously approved these changes Apr 20, 2021
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Apr 20, 2021
@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Apr 20, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2021

automerge label removed due to a CI failure

@mergify mergify bot dismissed mvines’s stale review April 20, 2021 07:14

Pull request has been modified.

@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Apr 20, 2021
@codecov
Copy link

codecov bot commented Apr 20, 2021

Codecov Report

Merging #16659 (77abffa) into master (01786f6) will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #16659   +/-   ##
=======================================
  Coverage    83.0%    83.0%           
=======================================
  Files         415      415           
  Lines      114995   115000    +5     
=======================================
+ Hits        95475    95480    +5     
  Misses      19520    19520           

@mergify mergify bot merged commit a7e65c0 into solana-labs:master Apr 20, 2021
mergify bot pushed a commit that referenced this pull request Apr 20, 2021
* RPC: use finalized as default pubsub commitment level

* update docs

* Fix tests

(cherry picked from commit a7e65c0)

# Conflicts:
#	core/tests/rpc.rs
mergify bot pushed a commit that referenced this pull request Apr 20, 2021
* RPC: use finalized as default pubsub commitment level

* update docs

* Fix tests

(cherry picked from commit a7e65c0)
@jstarry jstarry deleted the pubsub-finalized branch April 20, 2021 08:51
mergify bot added a commit that referenced this pull request Apr 20, 2021
* RPC: use finalized as default pubsub commitment level

* update docs

* Fix tests

(cherry picked from commit a7e65c0)

Co-authored-by: Justin Starry <justin@solana.com>
mergify bot added a commit that referenced this pull request Apr 20, 2021
… (#16665)

* RPC: use finalized as default pubsub commitment level (#16659)

* RPC: use finalized as default pubsub commitment level

* update docs

* Fix tests

(cherry picked from commit a7e65c0)

# Conflicts:
#	core/tests/rpc.rs

* fix conflicts

Co-authored-by: Justin Starry <justin@solana.com>
@brooksprumo brooksprumo mentioned this pull request Aug 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RPC: pubsub defaults to confirmed but rpc http defaults to finalized
2 participants