Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add transaction index in slot to geyser plugin TransactionInfo #25688

Merged
merged 17 commits into from
Jun 23, 2022

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented Jun 1, 2022

Problem

Geyser plugin receivers can't determine the order of signatures in a block; they receive signatures out of order because geyser streaming happens as transactions are processed in replay, which is done in parallel and with tx randomization.

Summary of Changes

Generate a list of transaction indexes in each slot, and include each index in a new ReplicaTransactionInfoVersion variant.
This will also enable reworking the Blockstore AddressSignatures column to include the transaction-index in the key, and fix the various getSignatureByAddress ordering/overlapping problems, pending follow-up

To do:

  • handle banking stage

@CriesofCarrots CriesofCarrots force-pushed the add-tx-index branch 4 times, most recently from e64f036 to 55867b6 Compare June 6, 2022 21:00
@CriesofCarrots CriesofCarrots marked this pull request as ready for review June 6, 2022 22:07
@CriesofCarrots CriesofCarrots requested a review from carllin June 6, 2022 22:07
@CriesofCarrots CriesofCarrots changed the title WIP: add transaction index in slot to geyser plugin TransactionInfo Add transaction index in slot to geyser plugin TransactionInfo Jun 7, 2022
rpc/src/transaction_status_service.rs Outdated Show resolved Hide resolved
poh/src/poh_recorder.rs Outdated Show resolved Hide resolved
poh/src/poh_recorder.rs Outdated Show resolved Hide resolved
core/src/banking_stage.rs Outdated Show resolved Hide resolved
poh/src/poh_recorder.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_processor.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_processor.rs Show resolved Hide resolved
ledger/src/blockstore_processor.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_processor.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_processor.rs Show resolved Hide resolved
@lijunwangs lijunwangs self-requested a review June 9, 2022 16:10
@CriesofCarrots CriesofCarrots force-pushed the add-tx-index branch 5 times, most recently from f07899b to f1fb849 Compare June 21, 2022 17:50
@CriesofCarrots CriesofCarrots requested a review from carllin June 21, 2022 19:37
poh/src/poh_recorder.rs Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jun 21, 2022

Codecov Report

Merging #25688 (bdaf8de) into master (1165a7f) will increase coverage by 0.0%.
The diff coverage is 93.1%.

❗ Current head bdaf8de differs from pull request most recent head 853b61b. Consider uploading reports for the commit 853b61b to get more accurate results

@@           Coverage Diff            @@
##           master   #25688    +/-   ##
========================================
  Coverage    81.9%    81.9%            
========================================
  Files         631      631            
  Lines      174252   174388   +136     
========================================
+ Hits       142728   142890   +162     
+ Misses      31524    31498    -26     

lijunwangs
lijunwangs previously approved these changes Jun 21, 2022
Copy link
Contributor

@lijunwangs lijunwangs left a comment

Choose a reason for hiding this comment

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

looks good to me

core/src/banking_stage.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_processor.rs Outdated Show resolved Hide resolved
ledger/src/blockstore_processor.rs Outdated Show resolved Hide resolved
@mergify mergify bot dismissed lijunwangs’s stale review June 22, 2022 19:31

Pull request has been modified.

@CriesofCarrots
Copy link
Contributor Author

All right, it's go time. Happy to do follow-ups, if you think of anything else, @carllin

@CriesofCarrots CriesofCarrots merged commit a6ba5a9 into solana-labs:master Jun 23, 2022
mergify bot pushed a commit that referenced this pull request Jun 23, 2022
* Define shuffle to prep using same shuffle for multiple slices

* Determine transaction indexes and plumb to execute_batch

* Pair transaction_index with transaction in TransactionStatusService

* Add new ReplicaTransactionInfoVersion

* Plumb transaction_indexes through BankingStage

* Prepare BankingStage to receive transaction indexes from PohRecorder

* Determine transaction indexes in PohRecorder; add field to WorkingBank

* Add PohRecorder::record unit test

* Only pass starting_transaction_index around PohRecorder

* Add helper structs to simplify test DashMap

* Pass entry and starting-index into process_entries_with_callback together

* Add tx-index checks to test_rebatch_transactions

* Revert shuffle definition and use zip/unzip

* Only zip/unzip if randomize

* Add confirm_slot_entries test

* Review nits

* Add type alias to make sender docs more clear

(cherry picked from commit a6ba5a9)

# Conflicts:
#	core/src/banking_stage.rs
#	ledger/src/blockstore_processor.rs
#	poh/src/poh_recorder.rs
gregcusack pushed a commit to gregcusack/solana that referenced this pull request Jun 23, 2022
…a-labs#25688)

* Define shuffle to prep using same shuffle for multiple slices

* Determine transaction indexes and plumb to execute_batch

* Pair transaction_index with transaction in TransactionStatusService

* Add new ReplicaTransactionInfoVersion

* Plumb transaction_indexes through BankingStage

* Prepare BankingStage to receive transaction indexes from PohRecorder

* Determine transaction indexes in PohRecorder; add field to WorkingBank

* Add PohRecorder::record unit test

* Only pass starting_transaction_index around PohRecorder

* Add helper structs to simplify test DashMap

* Pass entry and starting-index into process_entries_with_callback together

* Add tx-index checks to test_rebatch_transactions

* Revert shuffle definition and use zip/unzip

* Only zip/unzip if randomize

* Add confirm_slot_entries test

* Review nits

* Add type alias to make sender docs more clear
gregcusack pushed a commit that referenced this pull request Jun 29, 2022
* add three gossip metrics measuring gossip loop times

* add 5 metrics

* rm space

* rm space

* Update SECURITY.md

- fix nav link
- add bounty split policy for duplicate reports

* Add transaction index in slot to geyser plugin TransactionInfo (#25688)

* Define shuffle to prep using same shuffle for multiple slices

* Determine transaction indexes and plumb to execute_batch

* Pair transaction_index with transaction in TransactionStatusService

* Add new ReplicaTransactionInfoVersion

* Plumb transaction_indexes through BankingStage

* Prepare BankingStage to receive transaction indexes from PohRecorder

* Determine transaction indexes in PohRecorder; add field to WorkingBank

* Add PohRecorder::record unit test

* Only pass starting_transaction_index around PohRecorder

* Add helper structs to simplify test DashMap

* Pass entry and starting-index into process_entries_with_callback together

* Add tx-index checks to test_rebatch_transactions

* Revert shuffle definition and use zip/unzip

* Only zip/unzip if randomize

* Add confirm_slot_entries test

* Review nits

* Add type alias to make sender docs more clear

* Update SECURITY.md

finish filling out the table....

* rpc: fix possible deadlock in rpc (#26051)

* Add StatusCache::root_slot_deltas() and use it (#26170)

* Remove InMemAccountsIndex::map() and use map_internal directly (#26189)

* [quic]Decrement total_streams correctly (#26158)

* remove comment

* alphabetical metrics. no abbreviations

* remove trailing white space

* cargo fmt to update code format/readability

Co-authored-by: Trent Nelson <[email protected]>
Co-authored-by: Tyera Eulberg <[email protected]>
Co-authored-by: Boqin Qin(秦 伯钦) <[email protected]>
Co-authored-by: Brooks Prumo <[email protected]>
Co-authored-by: Miles Obare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants