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!: change monero consensus encoding an update for hardfork v15 #4492

Merged

Conversation

SWvheerden
Copy link
Collaborator

@SWvheerden SWvheerden commented Aug 17, 2022

Description

Monero-rs has sealed their encoding in the newer version. This means we need to encode the struct using our own encoding trait and only using the encoding for the monero versions.
Upgrade to the newest monero v15 hardfork code.

Also point towards our own monero-rs fork so we can update code when we want to.

@stringhandler stringhandler changed the base branch from development to testnet-dibbler August 18, 2022 07:07
@stringhandler stringhandler changed the base branch from testnet-dibbler to development August 18, 2022 07:07
@SWvheerden SWvheerden force-pushed the sw_change_monero_con branch 2 times, most recently from c1e5ecd to f7115d1 Compare August 18, 2022 12:23
@SWvheerden SWvheerden changed the title WIP feat!: change monero consensus encoding feat!: change monero consensus encoding Aug 18, 2022
@SWvheerden SWvheerden changed the title feat!: change monero consensus encoding fix!: change monero consensus encoding an update for hardfork v15 Aug 18, 2022
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

LGTM - minor comments so far. Still want to look a little closer

let len = VarInt::consensus_decode(d)?.0 as usize;
impl ConsensusDecoding for FixedByteArray {
fn consensus_decode<R: Read>(reader: &mut R) -> Result<Self, io::Error> {
let mut buf = [0u8; 1];
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified a bit (decodes from slice/vec encoding)

  let len = u64::consensus_decode(reader)? as usize;
        if len > MAX_ARR_SIZE {
            return Err(io::Error::new(
                io::ErrorKind::InvalidInput,
                format!("length exceeded maximum of {}-bytes for FixedByteArray: {}", MAX_ARR_SIZE, len),
            ));
        }
        let mut buf = [0u8; MAX_ARR_SIZE];
        reader.read_exact(&mut buf[..len])?;

        Ok(FixedByteArray{
            elems: buf,
            len: len as u8,
        })

stringhandler
stringhandler previously approved these changes Aug 19, 2022
@@ -54,7 +54,7 @@ integer-encoding = "3.0.2"
lmdb-zero = "0.4.4"
log = "0.4"
log-mdc = "0.1.0"
monero = { version = "^0.13.0", features = ["serde_support"], optional = true }
monero = { git = "https://github.com/tari-project/monero-rs.git", features = ["serde"], optional = true }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This must have a tag or a commit specified

fmt

Update base_layer/core/src/proof_of_work/monero_rx/fixed_array.rs

Co-authored-by: Stan Bondi <sdbondi@users.noreply.github.com>
fix clippy
@stringhandler stringhandler merged commit 2a3af27 into tari-project:development Aug 19, 2022
sdbondi added a commit to sdbondi/tari that referenced this pull request Aug 22, 2022
* development:
  feat!: add hashing API use to base layer (see issue tari-project#4394) (tari-project#4447)
  feat(wallet): adds --stealth-one-sided flag to make-it-rain (tari-project#4508)
  fix!: change monero consensus encoding an update for hardfork v15 (tari-project#4492)
  fix(core/covenants)!: update covenants to support OutputType enum (tari-project#4472)
  feat(core)!: restrict output types to only those permitted by consensus (tari-project#4467)
@SWvheerden SWvheerden deleted the sw_change_monero_con branch August 22, 2022 07:13
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