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(core)!: add missing consensus encoding length byte rangeproof & covenants #3730

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Jan 21, 2022

Description

  • Adds consensus encode impl for RangeProof and ComSig
  • Adds length varint for covenant
  • Move version as first encoded byte
  • Implement zero alloc hashing for outputs and witness hash
  • Limit covenant and tariscript byte size
  • Migration to set length byte in existing wallet transactions
  • Ensure that an RPC message can never be zero length.
  • covenant serde implementation does not use consensus encoding

Motivation and Context

Rangeproof is not fixed size and so requires a length varint.

It is important to ensure that a remote peer cannot cause unbounded allocation, so various length checks are implemented.

The wallet previously only ever stored a 0-byte covenant which is now invalid encoding.

Potential fix for WriteZero, ensuring that all rpc messages are non-zero in length (rare, but possible).

How Has This Been Tested?

Existing tests updated, manually - mined blocks on dibbler

@sdbondi sdbondi force-pushed the core-consensus-encode-witness branch 11 times, most recently from 20f8c60 to 1b531e4 Compare January 22, 2022 10:42
@sdbondi sdbondi force-pushed the core-consensus-encode-witness branch from 1b531e4 to efa7bb5 Compare January 23, 2022 12:33
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

This looks good. Made a comment or two.
We can remove all the places where we split the comsig's and then encode them as this pr now adds an implementation to implement them directly.

I also think we should use consensus encoding everywhere in the hashes and challenges as this is committed to on the blockchain, either in a MMR root, or just straight up in the output/input. And if we want another implementation to be able to verify this it needs to be able to decode/encode it.

@sdbondi sdbondi force-pushed the core-consensus-encode-witness branch 4 times, most recently from a864af3 to a44fd56 Compare January 24, 2022 07:04
@sdbondi sdbondi force-pushed the core-consensus-encode-witness branch from a44fd56 to 84d7b8e Compare January 24, 2022 07:20
@aviator-app aviator-app bot removed the mq-failed label Jan 25, 2022
@aviator-app aviator-app bot merged commit d56da1a into tari-project:development Jan 25, 2022
@sdbondi sdbondi deleted the core-consensus-encode-witness branch January 25, 2022 09:11
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