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

Fix consensus ordering for cross-chains #275

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

dr-orlovsky
Copy link
Member

@dr-orlovsky dr-orlovsky commented Sep 29, 2024

Implementation of RCP-20241013A

Alternative to #274 which takes care of situations where

  • blocks on testnet have timestamps which ordering is not aligned with the height
  • two different chains are used, and one doesn't report timestamps properly

@dr-orlovsky dr-orlovsky added the bug Something isn't working label Sep 29, 2024
@dr-orlovsky dr-orlovsky added this to the v0.11.0 milestone Sep 29, 2024
Copy link

codecov bot commented Sep 29, 2024

Codecov Report

Attention: Patch coverage is 70.49180% with 18 lines in your changes missing coverage. Please review.

Project coverage is 14.2%. Comparing base (7ae9760) to head (5ff9821).

Files with missing lines Patch % Lines
src/vm/contract.rs 70.5% 18 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##           master    #275     +/-   ##
========================================
+ Coverage    13.2%   14.2%   +1.0%     
========================================
  Files          29      29             
  Lines        3883    3938     +55     
========================================
+ Hits          513     558     +45     
- Misses       3370    3380     +10     
Flag Coverage Δ
rust 14.2% <70.5%> (+1.0%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@zoedberg zoedberg left a comment

Choose a reason for hiding this comment

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

two different chains are used, and one doesn't report timestamps properly

Could you please explain how does this PR address this?

src/vm/contract.rs Show resolved Hide resolved
testnet may have blocks which timestamps are not aligned with their heights
self.timestamp.cmp(&other.timestamp)
const BLOCK_TIME: i64 = 10 /*min*/ * 60 /*secs*/;
match (self.layer1, other.layer1) {
(a, b) if a == b => self.height.cmp(&other.height),
Copy link
Contributor

Choose a reason for hiding this comment

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

here #117 you said:

using block height and position in the block of the witness transaction

but this code isn't doing this, is the code incomplete or the documentation old? To me it seems a rule we should keep, to allow ordering of TXs in the same block

Copy link
Member Author

@dr-orlovsky dr-orlovsky Oct 14, 2024

Choose a reason for hiding this comment

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

I do not get: the code is exactly compares the block height, i.e. is doing what is said

Copy link
Contributor

Choose a reason for hiding this comment

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

To me it seems it's comparing just the block height, not the position of the TX in the block

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I see now.

#117 is outdated. In reality it happened that it is impossible to use position in the block since it is hard to compute from indexer's data. So instead we have started using opid value, which can be seen from the OpOrd structure (the ultimate operation ordering) below in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain how the OpId helps ordering TXs mined in the same block? From what I see the OpId is the commitment hash of the RGB operation, it seems unrelated to the mining status of the TX.

Copy link
Member Author

@dr-orlovsky dr-orlovsky Oct 15, 2024

Choose a reason for hiding this comment

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

We need to order operations, not transactions. We just utilize anchors and witness transaction to order operations, but we are not locked to just transaction information.

Since witness transactions may contain multiple operations (two anchors, a bundle per anchor, multiple state transitions per bundle, plus arbitrary large number of state extensions) txid or inside-block ordering won't help anyway

Copy link
Contributor

Choose a reason for hiding this comment

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

Still I don't understand how the OpId should allow a meaningful ordering of operations, since its just a hash. Could you please clarify this?

Copy link
Member Author

@dr-orlovsky dr-orlovsky Oct 15, 2024

Choose a reason for hiding this comment

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

Opid is just one of the factors of ordering. You asked "why we do not account for block position of witness transaction" and the answer here is "we do not need it".

Consensus ordering of operations takes many factors (listed in their priority):

  1. Blockchain
  2. Block height (or timestamp, when compared cross-chain in large window)
  3. Operation type (extensions go in front of transitions)
  4. Nonce
  5. Opid

1-2 are related to witness transactions, and 3-5 to operations themselves. Normally you use nonce (which we discussed a lot with you!) to do definite ordering within the same block! But if you misused nonce and gave two different state transition the same one, then (and only then!) opid comes into play. Yes, this is not a meaningful, but at least deterministic. For meaningful ordering one must use nonce properly.

src/vm/contract.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: In review
Development

Successfully merging this pull request may close these issues.

3 participants