Skip to content
This repository has been archived by the owner on Aug 23, 2020. It is now read-only.

validate bundles as they come in #1820

Open
wants to merge 11 commits into
base: white-flag
Choose a base branch
from

Conversation

acha-bill
Copy link
Contributor

Description

This adds a new QuickBundleValidation stage between Received and Broadcast stages.
If the tx submitted to this stage is solid & is a tail, it propagates it via TransactionValidator#addSolidTransaction

Fixes #1810

Type of change

  • Enhancement (a non-breaking change which adds functionality)

How Has This Been Tested?

  • Existing unit tests pass
  • Added difference scenarios that test edge cases for transaction solidity, tail and bundle validity.

Checklist:

  • My code follows the style guidelines for this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Gal Rogozinski and others added 11 commits January 5, 2020 13:43
* Fix python-regression utilities to support Python3

* Python3 'str' type does not support 'decode()' anymore, everything is utf-8 by default
* added bundle validity rules

* commented on test and added invalid bundle tests

* Fix: lower error severity for faulty neighbor to warning (iotaledger#1710)

Co-authored-by: Dyrell Chapman <[email protected]>
…r#1802)

* Enforce rules via flag

* make bundle validator more testable

* added test

* fix formatting

* fix mockito exception
Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Do you satisfy requirement 1 ("when a tail solidifies")?
Also tagging @DyrellC to have his opinion on how this will be done once his PRs are merged

@acha-bill
Copy link
Contributor Author

Currently, I'm counting only on quickSetSolid.
Should we call TransactionValidator#checkSolidity before proceeding?

@GalRogozinski
Copy link
Contributor

Hmm I think no...
What I meant is that you will also take into account the propagtion thread in TransactionValidator (it will soon be moved to TransactionSolidifier in Dyrell's PR)

The problem with calling checkSolidity is that it opens up an attack vector by requesting txs that do not exist...

Even though that if you read the #protocol channel on discord you will see I am thinking of patching 1.9.0 with this

@DyrellC
Copy link
Contributor

DyrellC commented Apr 2, 2020

As it is right now it does add tails to the propagation queue as per

if (tvm.isSolid() && tvm.getCurrentIndex() == 0) {
    List<TransactionViewModel> bundleTransactions = bundleValidator.validate(tangle, true,
                        snapshotProvider.getInitialSnapshot(), tvm.getHash());
    if (!bundleTransactions.isEmpty()) {
        transactionValidator.addSolidTransaction(tvm.getHash());
    }
}

but it does not account for a transaction that solidifies at a later point. If we want to make sure we're catching every bundle as it's going through then it may make more sense to wait for 1805 and 1821 to get mergedd so that we have a queue for solidification. With that in place we can then have any transaction that makes it through checkSolidity it then performs a bundle validity check from the transaction solidifier while updating the state of the transactions and placing them into the propagation queue. If that's the approach we think makes the most sense, then replacing the transactionValidator.addSolidTransaction(tvm.getHash()) will need to change to transactionSolidifier.addToSolidificationQueue(tvm.getHash()), and if that transaction gets solidified quickly (which most should) it will then be added to the propagation thread anyways. This would mean that we will need to update the solidifier logic so that when it gets to the update phase, we should be running a bundle validity check on the original transaction before placing the transaction into the propagation thread.

@GalRogozinski GalRogozinski changed the base branch from dev to white-flag April 2, 2020 15:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate bundles as they come in
4 participants