Do not merge: We need to make an approach to verification

[sinitcin]

This description is still approximate and not accurate, we need to define an approach and agree on checks. DO NOT INSPECT THE CODE. LET'S JUST DISCUSS.

Draft TransactionVerifier

I suggest adding a structure that will contain:

pub struct TransactionVerifier<'a, T: frame_system::Config> {
    // Pointer to a tx that we have to check 
    tx: &'a TransactionFor<T>,
    // All inputs, to avoid repeated search in the loop 
    all_inputs_map: BTreeMap<TokenId, TransactionOutputFor<T>>,
    // All outputs, to avoid repeated search in the loop 
    all_outputs_map: BTreeMap<TokenId, TransactionOutputFor<T>>,
    // Using TokenId, you can get the entire amount of this token in all inputs 
    total_value_of_input_tokens: BTreeMap<TokenId, TransactionOutputFor<T>>,
    // Using TokenId, you can get the entire amount of this token in all outputs 
    total_value_of_output_tokens: BTreeMap<TokenId, TransactionOutputFor<T>>,
    // A set of transaction verification functions, this approach will allow you to remove unnecessary cycles, which will speed up the function 
    set_of_checks: Vec<&'a mut FnMut(...)>,
    // ...
    // I may add a priority field to the set of checks. I'm still thinking here. 
}

This struct we will use this way in the pallet utxo:

    pub fn validate_transaction<T: Config>(
        tx: &TransactionFor<T>,
    ) -> Result<ValidTransaction, &'static str> {
        TransactionVerifier::<'_, T>::new(tx)
            .checking_inputs()
            .checking_outputs()
            .checking_utxos_exists()
            .checking_signatures()
            .checking_tokens_transferring()
            .checking_tokens_issued()
            .checking_nft_mint()
            .checking_assets_burn()
            .calculating_reward()
            .collect_result()?
    }

When creating a new instance of this structure, we must initialize the fields.

Each subsequent check adds a new instance of the function to set_of_checks, which will be called in collect_result.

At the moment we can split the verification function for these parts:

• checking_inputs

  • Checks that inputs exist in a transaction
  • Checking that the number of inputs is not more than the maximum allowed number, now in the code I see that it is u32::MAX
  • Ensure each input is used only a single time

• checking_outputs

  • Checks that outputs exist in a transaction
  • Checking that the number of outputs is not more than the maximum allowed number, now in the code I see that it is u32::MAX
  • Ensure each output is unique
  • Output value must be nonzero
  • An output can't exist already in the UtxoStore

• checking_utxos_exists

  • Resolve the transaction inputs by looking up UTXOs being spent by them.

• checking_signatures

  • if all spent UTXOs are available, check the math and signatures

• checking_tokens_transferring

  • We have to check that the total sum of input tokens is less or equal to output tokens. (Or just equal?)
  • All inputs with such data code must be correctly mapped to outputs
  • If NFT is sent we must not burn or lose data

• checking_tokens_issued

  • We must check the correctness of the issued tokens
  • We have to check the length of metadata_uri and ticker
  • We must check the correctness of value and decimal

• checking_nft_mint

  • We have to check the uniqueness of digital data, only one NFT can refer to one object
  • We have to check the length of metadata_uri

• checking_assets_burn

  • Is there burn more than possible?
  • Is there tocken_id exist for the burn?

• calculating_reward

  • Just collecting MLT for a transaction reward.

• collect_result

  • Call all of these functions in one loop.

Questions

• Do we need other checks?
• What is we need for checking Bitcoin Script?
• What is we need for checking contracts?
• If we can check an output address here, and add a possibility to find in the UtxoStore by any address format, then we can remove fn pick_utxo and fn send_to_address. Isn't that?

I'm glad to see any suggestions or critics.

[altonen]

I've understood that the reward can be paid in any token, is that still correct or is that a different kind of reward?

Btw, how do the code changes you've introduced in this PR relate to this proposal?

[sinitcin]

I've understood that the reward can be paid in any token, is that still correct or is that a different kind of reward?

The last thing that I heard, for the test-net we should use only MLT as a reward.

Btw, how do the code changes you've introduced in this PR relate to this proposal?

Added the comment at the top. We need just discus over here.

[iljakuklic]

Thanks for the write-up. In general, the breakdown into smaller validation steps you posted looks fine to me. I have, however, a couple of concerns about this approach. In particular, the TransactionVerifier type imposes a couple of restrictions on the implementation.

First, values for some of the fields are not available from the start so we would have to come up with default values or them. This is error prone because the validation methods may accidentally rely on fields that have not been properly populated yet at the point. Second, the validation methods are passed more data than necessary. For example, the reward calculation only depends on the total input amounts and total output amounts, other fields are not relevant. It is a good practice to only let functions depend on information that they require.

As a first step, I'd propose a simpler approach of just splitting the big monolithic function into a bunch of smaller ones, roughly like so:

fn validate_transaction(tx) -> Result<ValidTransaction, &str> {
  let output_total = check_outputs(tx.outputs)?;
  let utxos = lookup_utxos(tx.inputs)?;
  let input_total = check_inputs(tx.inputs, utxos)?;
  check_destinations(tx.inputs, utxos)?;
  let reward = calculate_reward(input_total, output_total).ok_or("sum outputs > sum inputs")?;
  ensure!(check_time_lock(tx), "too soon");
  // ... more stuff
  ValidTransaction { tx, ... }
}

All these functions return either Result or Option or bool to indicate validations failure. The latter two have to be mapped to the proper error with .ok_or or ensure!, respectively.

GIven approaching testnet release, we need to think about how to get there without much disruption. Something like the following protocol may be reasonably good:

• Before testnet launch
  • If you are implementing a new feature that needs an extra validation step, put it in a separate function and call it from validate_transaction
  • If you need to significantly modify one or more of the validation steps, use that as an opportunity to take the code block out into a separate function
• Shortly after testnet launch
  • Factor out the remaining validation steps into separate functions
• In the long run
  • We can have something approaching the design outlined here, preferably with type-level distinction between transactions at different stages of validation. The validation step takes RawTransaction to Transaction and all other functions taking transactions signal whether they assume transaction has already been validated by taking an argument of appropriate type. (This is only a very rough sketch to illustrate the high level idea, concrete implementation needs some refinement.)

Thoughts/comments?

[sinitcin]

Thoughts/comments?

Thank you, Lukas! To be honest, Ben and Sam have already mentioned that we do have no time for huge refactoring of this place. And today, I start splitting it almost like you explain.

For the long run, I thought we might make priorities for each checking fn and then call them depending on it. But implementing stages of validation looks better.

[TheQuantumPhysicist]

Commenting on what Lukas said:

First, values for some of the fields are not available from the start so we would have to come up with default values or them.

This is what I meant when I said in the standup that this is "stateful programming". Generally speaking, stateful programming is the paradigm where you initialize a state and keep updating it. It's basically what "object oriented programming" tries to do. Unfortunately, most of the time in security applications, this is the worst way of doing things unless there's a good reason to do it that way. Why? Because testing such a design is basically hell on earth. There's theoretically an infinite possible combinations of how the fields of such a complex class/struct can be populated, in both order and values. You can never test everything!

The better way is what Lukas mentioned. It's a form of functional programming. In functional programming, your program consists of inputs that go into functions, and outputs of these functions that go into other functions, and all functions are preferably pure (i.e., shouldn't depend on a global state). This way, we can test every individual functions through all possible inputs and outputs as much as possible, and everything is bounded in terms of possible states, and hence manageable.

Thank you guys for the effort and discussion, and like Anton mentioned, this is not something we're gonna do now anyway. But I appreciate the initiative and efforts.

[sinitcin]

Thank you guys for the effort and discussion, and like Anton mentioned, this is not something we're gonna do now anyway. But I appreciate the initiative and efforts.

Thank you too! Tokens would be impossible without your patronage.