checkConsistency throws "tails are not solid" few minutes after issuing a transaction

[viossat]

Bug description

Calling checkConsistency on a 25 minutes old transaction usually leads to a "tails are not solid" response. Related to iotaledger/trinity-wallet#1900

IRI version

1.8.1

Steps to reproduce

1. Send a new transaction
2. Wait 22-25 minutes
3. Call checkConsistency on that transaction

   Script to reproduce

   
   const { composeAPI } = require('@iota/core');
   const iota = composeAPI({provider: 'http://node-with-pow:14265'});

(async () => { const trytes = await iota.prepareTransfers('9'.repeat(81), [{address: '9'.repeat(81), value: 0}]); const txs = await iota.sendTrytes(trytes, 3, 14); const hash = txs[0].hash;

console.info(hash); console.info('Waiting for error...'); const start = Date.now(); setInterval(() => { iota.checkConsistency(hash, {rejectWithReason: true}).catch(err => { const end = Date.now(); console.error('Error after ' + (Math.floor((end - start) / 1000 / 60 * 10) / 10) + ' minutes'); console.error(err.message); process.exit(1); }); }, 1000); })();

### Expected behaviour
```json
{
    "state": true,
    "info": ""
}

Actual behaviour

{
    "state": false,
    "info": "tails are not solid (missing a referenced tx): <HASH>"
}

[GalRogozinski]

@viossat http://node-with-pow:14265 - did you use a node behind a load-balancer?

[viossat]

@GalRogozinski I tried with some nodes from the iota-nodes.net list, excluding the known load-balancers. It always reached "tails are not solid" within 22-25 minutes.

[GalRogozinski]

The results of the ICC tests explain the issue I would say

[DyrellC]

The reason for this is that in the checkConsistencyStatement() call there is a call to the walkValidator

 // Transactions are valid, lets check ledger consistency
        if (state) {
            snapshotProvider.getLatestSnapshot().lockRead();
            try {
                WalkValidatorImpl walkValidator = new WalkValidatorImpl(tangle, snapshotProvider, ledgerService, configuration);
                for (Hash transaction : transactions) {
                    if (!walkValidator.isValid(transaction)) {
                        state = false;
                        info = "tails are not consistent (would lead to inconsistent ledger state or below max depth)";
                        break;
                    }
                }
            } finally {
                snapshotProvider.getLatestSnapshot().unlockRead();
            }
        }

I'm not sure I understand fully the reasoning behind this logic as it will always return false for being below max depth once you cross the 15 milestone threshold. Even if the rest of the checks pass muster, this one will always fail and will always return false for older transactions.

@GalRogozinski perhaps you have a particular reason as to why this behaviour was introduced?

[GalRogozinski]

This is a consensus rule that was added to the tangle. It protects the tangle from DOS attacks while doing a RW. Below maxDepth atm checks 2 things:

1. That you are not referencing transactions that only confirm very old milestones (that are more than 15 indexes long)
2. That you are not referencing a huge subtangle. Since a traversal through its balance diff and signatures will exhaust our resources and DOS our node.

Now check 2 was put in place later than the original check 1. It was in order to mitigate the infamous side tangle attack (see #846). To be honest I am currently unsure why we need both checks 1 & 2.

According to our official documents, this is the purpose of check 1:

It boils down to this: if two transactions’ histories are too different, they should not be merged. If we let them merge, at some point it becomes too expensive to go through the history diff, validate all the signatures, and compute the ledger state.

However, now thinking it over, I think the new check 2 suffices...

I'll investigate the matter a bit more...

[GalRogozinski]

A problem that we may have from long range approvals stems from local snapshot feature. Currently local snapshot depth is at least 100. So this shouldn't be a problem. However someone can still make transaction that can reference the latest milestone and something in the far past...

Another problem is a long range attack, but on IOTA with coordinator this matters less. Compass will always walk from the depth we tell him.

I think we should try to come up with some attack scenarios to make sure there is no problem with removing criteria (1).

However, I am pretty sure that for checkConsistency we don't need it

[GalRogozinski]

So in short, let's make a PR that only get rids of check (1) in checkConsistency but leaves it in the WalkValidator for tip selection.

Maybe later on we remove it entirely but let's resolve this issue first

[GalRogozinski]

Implementation detail: consider extending walkValidator Impl

[acha-bill]

Because the static TransactionViewModel.fromHash is not mocked, tests like failOnBelowMaxDepthDueToOldMilestone
assert false postiives because they prematurely return false at the check solidity stage of isValid()

//mock
tx.updateSolid(true);

//isValid
tx = TransactionViewModel.fromHash(tangle, hash)
tx.isSolid() -> false //return tx is not solid 
//below max depth code never runs

We can use PowerMockito to mock the static methods here

@RunWith(PowerMockRunner.class)
@PrepareForTest(TransactionViewModel.class)
class WalkerValidatorNoMaxDepthImplTest {

    tx = TransactionTestUtils.createBundleHead(0);
    //set what we want
    tx.updateSolid(true)

    PowerMockito.mockStatic(TransactionViewModel.class);
    Mockito.when(TransactionViewModel.fromHash(tangle, hash)).thenReturn(tx) //return what we set

    Assert.assertTrue("failed", TransactionViewModel.fromHash(tangle,hash).isSolid())

    //Will now pass isSolid() check in `WalkValidatorImpl.isValid`
}

[GalRogozinski]

Hmmm,

but leaves it in the WalkValidator for tip selection

This requests meant that you should keep the current functionality for tip selection, and just change it for the api call...

Meaning that I'd rather see current tests not change too much.

You probably need to add new tests, you can use PowerMockito if you like

[GalRogozinski]

Trinity will check that the transaction is not too old by checking attachment timestamp. We may add a new isPromotable api call