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

Pull RpcSubscriptions out of the Bank#2806

Merged
garious merged 4 commits intosolana-labs:masterfrom
garious:boot-subscriptions
Feb 19, 2019
Merged

Pull RpcSubscriptions out of the Bank#2806
garious merged 4 commits intosolana-labs:masterfrom
garious:boot-subscriptions

Conversation

@garious
Copy link
Copy Markdown
Contributor

@garious garious commented Feb 17, 2019

Problem

Notifications are not fork-aware. We notify as soon as either a TPU or TVU commits transactions. Instead, we should notify on block boundaries and any time we switch forks (right after a vote).

Summary of Changes

Remove notifications from the bank and rewire the PubSub tests to be more unit-testy.

TODO:

  • Send notifications from the TVU

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 17, 2019

Codecov Report

Merging #2806 into master will decrease coverage by <.1%.
The diff coverage is 95%.

@@           Coverage Diff            @@
##           master   #2806     +/-   ##
========================================
- Coverage    77.4%   77.4%   -0.1%     
========================================
  Files         125     125             
  Lines       19048   19033     -15     
========================================
- Hits        14754   14736     -18     
- Misses       4294    4297      +3

@garious garious force-pushed the boot-subscriptions branch 2 times, most recently from 14ea25b to c235e83 Compare February 19, 2019 00:45
Pass in RpcSubscriptions instead, which let's you choose a
bank fork when it's time to send notifications.
@garious garious marked this pull request as ready for review February 19, 2019 02:12
@garious
Copy link
Copy Markdown
Contributor Author

garious commented Feb 19, 2019

@mvines, for apps like tic-tac-toe, this might not work quite as you'd expect until we start checkpointing on every block. Even so, if this change is remotely tolerable (get it?), then I'd like to merge it. I have bigger fish to fry. Maybe pull it down locally for a few sanity checks before this goes in?

Copy link
Copy Markdown
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

smells like progress, r+

@garious
Copy link
Copy Markdown
Contributor Author

garious commented Feb 19, 2019

@CriesofCarrots, feel free to review post-merge

@garious garious merged commit ad9cd23 into solana-labs:master Feb 19, 2019
jeffwashington added a commit to jeffwashington/solana that referenced this pull request Sep 4, 2024
* unref accounts in shink and pack when we're committed

* remove bad comments

* rewrite shrink_ancient_fail_ref test

* fix comments

* fix a test

* fix another test

* fmt

* del invalid comments

* reviews: move log to new PR.

* Revert "reviews: move log to new PR."

This reverts commit f8aefe0.

* fix comments

* revert log content

* pr: rename

* pr: more rename

---------

Co-authored-by: HaoranYi <haoran.yi@anza.xyz>
wen-coding pushed a commit to wen-coding/solana that referenced this pull request Sep 4, 2024
* unref accounts in shink and pack when we're committed

* remove bad comments

* rewrite shrink_ancient_fail_ref test

* fix comments

* fix a test

* fix another test

* fmt

* del invalid comments

* reviews: move log to new PR.

* Revert "reviews: move log to new PR."

This reverts commit f8aefe0.

* fix comments

* revert log content

* pr: rename

* pr: more rename

---------

Co-authored-by: HaoranYi <haoran.yi@anza.xyz>
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.

2 participants