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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/stl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub const LIB_ID_RGB_COMMIT: &str =
"stl:ZMTVCU25-QDo98xR-wI91wcu-ydb7kui-QfZbF$n-0KDS2ow#tuna-safari-design";
/// Strict types id for the library providing data types for RGB consensus.
pub const LIB_ID_RGB_LOGIC: &str =
"stl:bioTBozT-NqelHGE-SPbnpMA-XBNSbXZ-6X0dANE-WHVirL8#explain-marvin-bless";
"stl:IKFjGiNg-4MC4DKp-6XBvG0D-FCMXfqx-OnKhuRH-nb75Mcs#sector-season-anatomy";

fn _rgb_commit_stl() -> Result<TypeLib, CompileError> {
LibBuilder::new(libname!(LIB_NAME_RGB_COMMIT), tiny_bset! {
Expand Down
120 changes: 108 additions & 12 deletions src/vm/contract.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,23 +23,24 @@
use std::borrow::Borrow;
use std::cell::RefCell;
use std::cmp::Ordering;
use std::fmt::Debug;
use std::fmt::{self, Debug, Display, Formatter};
use std::num::NonZeroU32;
use std::rc::Rc;

use amplify::confinement;
use amplify::num::u24;
use bp::seals::txout::{CloseMethod, ExplicitSeal, VerifyError, Witness};
use bp::{dbc, Tx, Txid};
use chrono::{MappedLocalTime, TimeZone, Utc};
use commit_verify::mpc;
use single_use_seals::SealWitness;
use strict_encoding::{StrictDecode, StrictDumb, StrictEncode};

use crate::{
AssetTags, AssignmentType, Assignments, AssignmentsRef, AttachState, ContractId, DataState,
ExposedSeal, Extension, ExtensionType, FungibleState, Genesis, GlobalState, GlobalStateType,
GraphSeal, Impossible, Inputs, Metadata, OpFullType, OpId, OpType, Operation, Transition,
TransitionType, TxoSeal, TypedAssigns, Valencies, XChain, XOutpoint, XOutputSeal,
GraphSeal, Impossible, Inputs, Layer1, Metadata, OpFullType, OpId, OpType, Operation,
Transition, TransitionType, TxoSeal, TypedAssigns, Valencies, XChain, XOutpoint, XOutputSeal,
LIB_NAME_RGB_LOGIC,
};

Expand Down Expand Up @@ -290,29 +291,72 @@
}
}

#[derive(Copy, Clone, PartialEq, Eq, Hash, Debug, Display)]
#[derive(StrictType, StrictDumb, StrictEncode, StrictDecode)]
#[derive(Getters, Copy, Clone, PartialEq, Eq, Hash, Debug)]
#[derive(StrictType, StrictEncode, StrictDecode)]
#[strict_type(lib = LIB_NAME_RGB_LOGIC)]
#[cfg_attr(
feature = "serde",
derive(Serialize, Deserialize),
serde(crate = "serde_crate", rename_all = "camelCase")
)]
#[display("{height}@{timestamp}")]
pub struct WitnessPos {
height: u32,
#[getter(as_copy)]
layer1: Layer1,

// TODO: Move BlockHeight from bp-wallet to bp-consensus and use it here
#[getter(as_copy)]
height: NonZeroU32,

#[getter(as_copy)]
timestamp: i64,
}

impl StrictDumb for WitnessPos {
fn strict_dumb() -> Self {
Self {
layer1: Layer1::Bitcoin,
height: NonZeroU32::MIN,
timestamp: 1231006505,
}
}
}

// Sat Jan 03 18:15:05 2009 UTC
const BITCOIN_GENESIS_TIMESTAMP: i64 = 1231006505;

// Sat Jan 03 18:15:05 2009 UTC
const LIQUID_GENESIS_TIMESTAMP: i64 = 1296692202;

impl WitnessPos {
pub fn new(height: u32, timestamp: i64) -> Option<Self> {
if height == 0 || timestamp < 1231006505 {
#[deprecated(
since = "0.11.0-beta.9",
note = "please use `WitnessPos::bitcoin` or `WitnessPos::liquid` instead"
)]
pub fn new(height: NonZeroU32, timestamp: i64) -> Option<Self> {
Self::bitcoin(height, timestamp)
}

Check warning on line 337 in src/vm/contract.rs

View check run for this annotation

Codecov / codecov/patch

src/vm/contract.rs#L335-L337

Added lines #L335 - L337 were not covered by tests

pub fn bitcoin(height: NonZeroU32, timestamp: i64) -> Option<Self> {
if timestamp < BITCOIN_GENESIS_TIMESTAMP {
return None;
}
Some(WitnessPos { height, timestamp })
Some(WitnessPos {
layer1: Layer1::Bitcoin,
height,
timestamp,
})
}

pub fn height(&self) -> NonZeroU32 { NonZeroU32::new(self.height).expect("invariant") }
pub fn liquid(height: NonZeroU32, timestamp: i64) -> Option<Self> {
if timestamp < LIQUID_GENESIS_TIMESTAMP {
return None;
}
Some(WitnessPos {
layer1: Layer1::Liquid,
height,
timestamp,
})
}
}

impl PartialOrd for WitnessPos {
Expand All @@ -327,7 +371,31 @@
fn cmp(&self, other: &Self) -> Ordering {
assert!(self.timestamp > 0);
assert!(other.timestamp > 0);
self.timestamp.cmp(&other.timestamp)
const BLOCK_TIME: i64 = 10 /*min*/ * 60 /*secs*/;
zoedberg marked this conversation as resolved.
Show resolved Hide resolved
match (self.layer1, other.layer1) {
(a, b) if a == b => self.height.cmp(&other.height),

Check warning on line 376 in src/vm/contract.rs

View check run for this annotation

Codecov / codecov/patch

src/vm/contract.rs#L375-L376

Added lines #L375 - L376 were not covered by tests
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.

(Layer1::Bitcoin, Layer1::Liquid)
if (self.timestamp - other.timestamp).abs() < BLOCK_TIME =>
{
Ordering::Greater

Check warning on line 380 in src/vm/contract.rs

View check run for this annotation

Codecov / codecov/patch

src/vm/contract.rs#L378-L380

Added lines #L378 - L380 were not covered by tests
}
(Layer1::Liquid, Layer1::Bitcoin)
if (other.timestamp - self.timestamp).abs() < BLOCK_TIME =>
{
Ordering::Less

Check warning on line 385 in src/vm/contract.rs

View check run for this annotation

Codecov / codecov/patch

src/vm/contract.rs#L383-L385

Added lines #L383 - L385 were not covered by tests
}
_ => self.timestamp.cmp(&other.timestamp),

Check warning on line 387 in src/vm/contract.rs

View check run for this annotation

Codecov / codecov/patch

src/vm/contract.rs#L387

Added line #L387 was not covered by tests
}
}

Check warning on line 389 in src/vm/contract.rs

View check run for this annotation

Codecov / codecov/patch

src/vm/contract.rs#L389

Added line #L389 was not covered by tests
}

impl Display for WitnessPos {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
write!(f, "{}:{}, ", self.layer1, self.height)?;
match Utc.timestamp_opt(self.timestamp, 0) {
MappedLocalTime::Single(time) => write!(f, "{}", time.format("%Y-%m-%d %H:%M:%S")),
_ => f.write_str("invalid timestamp"),

Check warning on line 397 in src/vm/contract.rs

View check run for this annotation

Codecov / codecov/patch

src/vm/contract.rs#L393-L397

Added lines #L393 - L397 were not covered by tests
}
}
}

Expand Down Expand Up @@ -672,3 +740,31 @@
}
}
}

#[cfg(test)]
mod test {
use super::*;

#[test]
fn witness_post_timestamp() {
assert_eq!(WitnessPos::bitcoin(NonZeroU32::MIN, BITCOIN_GENESIS_TIMESTAMP - 1), None);
assert_eq!(WitnessPos::liquid(NonZeroU32::MIN, LIQUID_GENESIS_TIMESTAMP - 1), None);
assert_eq!(WitnessPos::liquid(NonZeroU32::MIN, BITCOIN_GENESIS_TIMESTAMP), None);
assert!(WitnessPos::bitcoin(NonZeroU32::MIN, BITCOIN_GENESIS_TIMESTAMP).is_some());
assert!(WitnessPos::liquid(NonZeroU32::MIN, LIQUID_GENESIS_TIMESTAMP).is_some());
assert!(WitnessPos::bitcoin(NonZeroU32::MIN, LIQUID_GENESIS_TIMESTAMP).is_some());
}

#[test]
fn witness_pos_getters() {
let pos = WitnessPos::bitcoin(NonZeroU32::MIN, BITCOIN_GENESIS_TIMESTAMP).unwrap();
assert_eq!(pos.height(), NonZeroU32::MIN);
assert_eq!(pos.timestamp(), BITCOIN_GENESIS_TIMESTAMP);
assert_eq!(pos.layer1(), Layer1::Bitcoin);

let pos = WitnessPos::liquid(NonZeroU32::MIN, LIQUID_GENESIS_TIMESTAMP).unwrap();
assert_eq!(pos.height(), NonZeroU32::MIN);
assert_eq!(pos.timestamp(), LIQUID_GENESIS_TIMESTAMP);
assert_eq!(pos.layer1(), Layer1::Liquid);
}
}
Loading