[HackerOne-2311934] Verify transactions prior to speculation

[raychu86]

Motivation

This PR performs transaction verification in VM::speculate and aborts the transactions that are invalid. This is necessary because in snarkOS when we construct subdags, we do not validate transmissions found in proposals, which could lead to block production halting if there are malicious validators. With the new change, we can eliminate the invalid transactions prior to block production and ensure that Block::check_next_block doesn't error.

Note that now we perform transaction verification prior to creating the block itself, rather than just before the block is about to be added to storage. Because of the transaction partial verification cache, we do not perform duplicate verification on the execution/deployment, but we do have to perform the (cheaper) uniqueness checks twice.

Related PRs

This PR is related to AleoHQ/snarkOS#3104.

And should replace: AleoHQ/snarkOS#3083 and AleoHQ/snarkOS#3098.

This should fix: AleoHQ/snarkOS#2991 and AleoHQ/snarkOS#2990.

[vicsn]

FYI when trying to compile this into snarkOS for testing I get the error:

error[E0038]: the trait `snarkos_node_bft_ledger_service::LedgerService` cannot be made into an object
   --> node/sync/src/block_sync.rs:118:49
    |
118 |     pub fn new(mode: BlockSyncMode, ledger: Arc<dyn LedgerService<N>>) -> Self {
    |                                                 ^^^^^^^^^^^^^^^^^^^^ `snarkos_node_bft_ledger_service::LedgerService` cannot be made into an object
    |
note: for a trait to be "object safe" it needs to allow building a vtable to allow the call to be resolvable dynamically; for more information visit <https://doc.rust-lang.org/reference/items/traits.html#object-safety>
   --> /Users/victorsintnicolaas/programs/snarkOS/node/bft/ledger-service/src/traits.rs:107:8
    |
107 |     fn prepare_advance_to_next_quorum_block<R: Rng + CryptoRng>(
    |        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait cannot be made into an object because method `prepare_advance_to_next_quorum_block` has generic type parameters

[ljedrz]

FYI when trying to compile this into snarkOS for testing I get the error:

As for the object safety issue, it's not ideal, but we could work around it by having 2 versions of prepare_advance_to_next_quorum_block, with TestRng hardcoded for #[cfg(test)] scenarios 🤔.

[vicsn]

This comment still applies: AleoHQ/snarkOS#3083 (comment)
I was able to kill all the nodes with some big batch proposals in a test suite.

With the system allocator (which is heavier than jemalloc):

• The maximum RAM to verify a 1M constraint program is 2GB.
• The maximum RAM for a 1 or 31-depth 1M constraint call execution is negligible, but to be safe we could assume 10MB

A thorough but brittle approach would be to estimate memory usage based on the number of constraints. But an easier setup would be a separate deploy and an execute verification maximum:

const MAX_PARALLEL_DEPLOY_VERIFICATIONS = 5;
const MAX_PARALLEL_EXECUTE_VERIFICATIONS = 1000;

let (deploys, executions) = candidate_transactions.partition(|tx|tx.is_deploy());
let deploy_iter = deploys.chunks(MAX_PARALLEL_DEPLOY_VERIFICATIONS);
let execution_iter = executions.chunks(MAX_PARALLEL_EXECUTE_VERIFICATIONS);
let tx_iter = deploy_iter.chain(execution_iter);

while let Some(txs) = tx_iter.next() {
    cfg_into_iter!(txs.zip(rngs).map(|| ...verify...)
}

[raychu86]

How is this different from the current approach of transaction verification on the current mainnet branch in check_next_block?

Are you saying that fundamentally the verification logic should be updated?

[vicsn]

Yes, that piece of parallel verification logic was prone to the same vulnerability.

[raychu86]

I've implemented the OOM fix here - #2387. If it is approved, we can apply the change to this PR as well.

[raychu86]

FYI when trying to compile this into snarkOS for testing I get the error:

@vicsn Similar to how we use a thread_rng for check_next_block and check_transaction_basic, this should allow testing to compile:

AleoHQ/snarkOS@mainnet...abort-malicious-txes

[vicsn]

@vicsn Similar to how we use a thread_rng for check_next_block and check_transaction_basic, this should allow testing to compile:

Thank you, will test it and report back.

[ghostant-1017]

Hi guys,
I was notified that [HackerOne-2310746/2311934] were duplicated. As I understand it and Report-2310746 focuses only on the Fee and is easier to fix, while Report-2311934 is more comprehensive. Will you reconsider the results of these two report assessments? Thank you! @vicsn @raychu86

[raychu86]

Hi guys, I was notified that [HackerOne-2310746/2311934] were duplicated. As I understand it and Report-2310746 focuses only on the Fee and is easier to fix, while Report-2311934 is more comprehensive. Will you reconsider the results of these two report assessments? Thank you! @vicsn @raychu86

Although both of these issues can be resolved with a single fix, in my opinion, they could be considered separate reports.

The snarkOS PR AleoHQ/snarkOS#3104, should address the Fee finding, and this PR should address both of the findings (for added safety).

[ghostant-1017]

Thank you! @raychu86