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

Improve btccheckpointinfo rpc #370

Merged
merged 3 commits into from
May 2, 2023
Merged

Conversation

KonradStaniec
Copy link
Collaborator

Improves BtcCheckpointInfo query:

  • 25bd33e - Removes redundant implementation of choosing best checkpoint submission, and uses function exposed by keeper to do the job
  • 1291798 - re-names fields to make more sense. It will affect any body who calls this endpoint, but current naming makes no sense and does not tell caller too much. @vitsalis if you think it is to late before testnet release to do this change I can remove this commit, but imo it needs to be done at some point.

As far as performance go this pr does not make any changes. Most probably biggest performance hit in this endpoint is the fact of linear scans when calling MainChainDepth. One hacky improvement would be to avoid calling MainChainDepth when epoch is finalized, as then we know for sure that submission is on main chain and deep enough so we could only call BlockHeight call, but I would avoid doing this if the performance unless it is critical.Imo it is better to wait for improved btc lightclient.

Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Nice! np about the changes will update relevant PRs

// the BTC checkpoint transactions on the above block
repeated TransactionInfo earliest_btc_block_txs = 4;
// the BTC checkpoint transactions of the best submission
repeated TransactionInfo best_submission_transactions = 4;
// list of vigilantes' addresses
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// list of vigilantes' addresses
// list of vigilantes' addresses of the best submission

// Index of the latest transaction in youngest submission block
LatestTxIndex uint32
YoungestBlockInitialTxIdx uint32
Copy link
Member

Choose a reason for hiding this comment

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

Why the initial keyword here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm yea it a bit weird. Maybe lowest will be better ? As this is only used for tie resolving, and this is transaction with lowest transaction idx in youngest block.

Copy link
Member

Choose a reason for hiding this comment

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

yep sounds good

@KonradStaniec KonradStaniec force-pushed the improve-btccheckpointinfo-rpc branch from 3ee8033 to e2bcae3 Compare May 2, 2023 09:43
@KonradStaniec KonradStaniec merged commit 13bb8a7 into dev May 2, 2023
@KonradStaniec KonradStaniec deleted the improve-btccheckpointinfo-rpc branch May 2, 2023 09:57
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.

2 participants