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

Transaction Validation #736

Open
wants to merge 24 commits into
base: develop
Choose a base branch
from

Conversation

SethDusek
Copy link
Collaborator

@SethDusek SethDusek commented Dec 28, 2023

Initial stateless validation work (things we can validate without needing blockchain context). Also includes a fix where we were allowing as many as u16::MAX inputs/outputs (65535) when ergo only allows i16::MAX (Short.MaxValue in scala) which is (32767). https://github.com/ergoplatform/ergo/blob/48239ef98ced06617dc21a0eee5670235e362933/ergo-core/src/main/scala/org/ergoplatform/modifiers/mempool/ErgoTransaction.scala#L93

…saction

Scala's Short.MaxValue is equivalent to the maximum a 16 bit signed integer can hold
@coveralls
Copy link

coveralls commented Dec 29, 2023

Pull Request Test Coverage Report for Build 8306068744

Details

  • 165 of 228 (72.37%) changed or added relevant lines in 15 files are covered.
  • 30 unchanged lines in 17 files lost coverage.
  • Overall coverage increased (+0.05%) to 80.801%

Changes Missing Coverage Covered Lines Changed/Added Lines %
ergo-lib/src/chain/transaction/ergo_transaction.rs 28 29 96.55%
ergotree-interpreter/src/eval/error.rs 0 2 0.0%
ergotree-interpreter/src/eval/scontext.rs 3 5 60.0%
ergotree-interpreter/src/eval/scoll.rs 8 11 72.73%
ergotree-ir/src/mir/value.rs 3 6 50.0%
ergo-lib/src/chain/transaction.rs 7 11 63.64%
ergo-lib/src/chain/transaction/storage_rent.rs 7 21 33.33%
ergo-lib/src/chain/parameters.rs 12 28 42.86%
ergo-lib/src/wallet/tx_context.rs 73 91 80.22%
Files with Coverage Reduction New Missed Lines %
ergotree-interpreter/src/sigma_protocol/prover.rs 1 78.43%
ergotree-ir/src/mir/avl_tree_data.rs 1 91.3%
ergo-chain-generation/src/chain_generation.rs 1 87.33%
ergotree-interpreter/src/eval/exponentiate.rs 1 81.82%
ergotree-ir/src/serialization/sigmaboolean.rs 1 81.63%
ergo-lib/src/wallet/tx_builder.rs 1 73.83%
ergo-lib/src/wallet/derivation_path.rs 1 79.17%
ergo-rest/src/api/peer_discovery_internals/non_chrome.rs 1 73.28%
ergo-lib/src/wallet/box_selector/simple.rs 1 78.1%
ergotree-interpreter/src/eval/deserialize_register.rs 1 79.17%
Totals Coverage Status
Change from base Build 7307508480: 0.05%
Covered Lines: 10719
Relevant Lines: 13266

💛 - Coveralls

set_hook can only be called once, this means displaying an error multiple times causes a panic in sigma-rust. ignoring seems like the best solution
When converting a signed integer i8 like -1 (0xff) to a bigger signed integer like i64, the 2's complement representation will be padded with ff (0xffffffff...). Thus if input[7] for example is -1, when it is |'ed, all high bits will be set to 1. Converting u8 0xff to i64 0xff leads to correct conversion (0x000000...ff) This fixes validation for transactions like https://ergexplorer.com/transactions#7f838c5060185125cb554323401d87c0fc135a0c5fc545f9e315ffc38b97b06e
The current indexOf implementation doesn't match the behavior of the one in sigmastate-interpreter. The previous version wrongly treated the second argument as a fallback index whereas it is actually the position in the array to start searching from
Some mainnet transactions like 9f4d40607a561c14a504c96919362f150fca3337ab43219d9a683f1617b74b0a have proofs that are smaller than usual but still valid. Relevant sigmastate-interpreter commit ScorexFoundation/sigmastate-interpreter@ef6ff08
In v5.0 onwards flatMap can have any arbitrary lambda in body as long as it returns a Coll[T]
This brings down TransactionContext creation for transactions with 32767 inputs down from ~1.2 seconds to ~160 milliseconds
This reduces validation time for some transactions like 34fecabe033ca5b516ddaf99a251baf1a580db5be1ccd7ca41bfcd953bb1923f from 11 seconds down to ~1.2
@SethDusek SethDusek force-pushed the txvalidation branch 4 times, most recently from 6a556e1 to 7529e70 Compare March 15, 2024 04:41
Dependencies and rust-analyzer are broken on older rust version. Also bump hashbrown due to ahash's nightly features breaking on older versions
@SethDusek SethDusek marked this pull request as ready for review March 18, 2024 11:14
@SethDusek
Copy link
Collaborator Author

SethDusek commented Mar 18, 2024

Should be good to go now! Some notes:

  • This PR despite the usual unit tests was tested using ergovalidation which downloads transactions from node (requires extra indexing to be enabled) and validates them. After fixing some script bugs we're now down to 15 transactions on mainnet that sigma-rust rejects, all of which rely on buggy behavior from v4.0 that no longer holds in v5.0, including CONTEXT.selfBoxIndex being -1 and sigmaProp(sigmaProp(true)) being allowed. I believe fixing these (and adding version-specific actions to ergotree-interpreter) is out of scope for this PR. Gist of failing transactions

  • I had to bump rust version since sigma-rust no longer builds as some dependencies don't support rust 1.64 anymore. Clippy rejection seems to be related and should be ignored for now

  • There are some limitations still in interpreter. The biggest one is that atLeast(0, ...) isn't supported (I plan on addressing this separately in another PR, but I fixed atLeast(1, ...) not being supported in this PR).

  • There are some breaking changes in this PR, namely ErgoStateContext now also requiring Parameters. I believe this is an acceptable trade-off even if parameters aren't needed for signing. Later when sigma-rust adds support for costing we could use Parameters to calculate costs during signing

  • Also fixed quadratic scaling in TransactionContext::new
    old:
    txcontextold
    new:
    txcontextnew

@@ -24,15 +24,20 @@ pub(crate) static INDEX_OF_EVAL_FN: EvalFn = |_env, _ctx, obj, args| {
.get(0)
.cloned()
.ok_or_else(|| EvalError::NotFound("indexOf: missing first arg".to_string()))?;
let fallback_index = args
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SethDusek SethDusek assigned SethDusek and unassigned SethDusek Apr 25, 2024
/// When using sigma-rust where heights are always unsigned, this error may be because creation height was set to be >= 2147483648
NegativeHeight,
#[error("Output box size {0} > maximum {}", ErgoBox::MAX_BOX_SIZE)]
/// Box size is > [ErgoBox::MAX_SCRIPT_SIZE]
Copy link
Member

Choose a reason for hiding this comment

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

MAX_BOX_SIZE

@kushti
Copy link
Member

kushti commented Apr 26, 2024

Please fix linter issues

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants