Lisk-sdk: Transaction pool behavior breaks backward compatibility causing hard-fork

[4miners]

Expected behavior

Transactions should be processed the same way as in the previous versions (for example 1.6.0) so backward compatibility is preserved.

Actual behavior

Transactions are processed differently (or in different order) resulting with breaking backward compatibility with older versions (hard-fork).

Suppose the account have 1 LSK balance and we sent following transactions in this particular order:

1. Spend from the account 0.5 LSK + 0.1 LSK fee
2. Spend from the account 0.5 LSK + 0.1 LSK fee
3. Credit the account with 0.3 LSK

Last 3 transactions are included in the same block. When older nodes are processing this block they apply those transactions in order 1 - (0.5 + 0.1) - (0.5 + 0.1) + 0.3 - this is not possible because account balance cannot go negative.

[ERR] 2019-07-03 10:16:08 | Failed to apply transaction: 1485790066807584278 to unconfirmed state of account - Account does not have enough LSK: 8772818227501265890L balance: 0.4
[ERR] 2019-07-03 10:16:08 | Transaction - {"id":"1485790066807584278","height":5962,"type":0,"timestamp":98038652,"senderPublicKey":"21cae3b2639038d77fe8fc6bba2a27c0ac3e9dead18d8d349aadb9e3c7d798a6","senderId":"8772818227501265890L","recipientId":"13604394588210584148L","amount":"50000000","fee":"10000000","signature":"5ff6347e9d4e17635e7b764254a28b4b66e0e2aa4014191cd98df1c8066b945defe805db8a1029c244a705ef6f4033f2f6977b136931c4a766920738d0f4d807","signatures":[],"confirmations":null,"asset":{},"blockId":"3406159478353082838","ready":true}
[dbg] 2019-07-03 10:16:08 | Block processing failed - {"id":"3406159478353082838","err":"Error: Failed to apply transaction: 1485790066807584278 to unconfirmed state of account - Account does not have enough LSK: 8772818227501265890L balance: 0.4","module":"blocks","block":{"id":"3406159478353082838","version":1,"timestamp":98038660,"height":5962,"previousBlock":"16526343828936722422","numberOfTransactions":3,"totalAmount":"130000000","totalFee":"30000000","reward":"500000000","payloadLength":351,"payloadHash":"74052bd734cc960b40eb68c225bf68b49881c5e9cb862492a9ba2a00d2c5bd9b","generatorPublicKey":"fab7b58be4c1e9542c342023b52e9d359ea89a3af34440bdb97318273e8555f0","generatorId":"9528507096611161860L","blockSignature":"f8bc80fa0244905a83e467cd205d2a89c9bb741999e97d24367dd9ee5011d0b9f9916b7d4324ce52111039ab9993610a87542a4370668658c44a8d476797b608","confirmations":null,"totalForged":"530000000","transactions":[{"id":"17565598885086760784","height":5962,"type":0,"timestamp":98038652,"senderPublicKey":"12c23f3942907327d5d6422f069f13f056ae9ab6752fc2c2344075abec03a813","senderId":"7069564171607021764L","recipientId":"8772818227501265890L","amount":"30000000","fee":"10000000","signature":"b9bbff723bbff9204aee2b8ebb582e70b6c5104738bc0f7fd19b8d9ac308ee85a62bc9b3e5e3190738ac61c5328acc686ff6ff542e6c68d72e9e4816cc528803","signatures":[],"confirmations":null,"asset":{},"blockId":"3406159478353082838","ready":true},{"id":"9239255295860703076","height":5962,"type":0,"timestamp":98038652,"senderPublicKey":"21cae3b2639038d77fe8fc6bba2a27c0ac3e9dead18d8d349aadb9e3c7d798a6","senderId":"8772818227501265890L","recipientId":"4547759333925451495L","amount":"50000000","fee":"10000000","signature":"ee1d52de8c3d6bbd3b10f7803df99e5e9c64c0936b8fe5487a3f870dcfb3ae8abff9487729c4fce1137db8527d4bca30b66adef0788aa56bd19136aafe89ce04","signatures":[],"confirmations":null,"asset":{},"blockId":"3406159478353082838","ready":true},{"id":"1485790066807584278","height":5962,"type":0,"timestamp":98038652,"senderPublicKey":"21cae3b2639038d77fe8fc6bba2a27c0ac3e9dead18d8d349aadb9e3c7d798a6","senderId":"8772818227501265890L","recipientId":"13604394588210584148L","amount":"50000000","fee":"10000000","signature":"5ff6347e9d4e17635e7b764254a28b4b66e0e2aa4014191cd98df1c8066b945defe805db8a1029c244a705ef6f4033f2f6977b136931c4a766920738d0f4d807","signatures":[],"confirmations":null,"asset":{},"blockId":"3406159478353082838","ready":true}]}}

Steps to reproduce

Send spend transaction(s) that causing account balance to go negative and credit transaction(s) that makes account balance to go positive again. Those needs to be included in one block.

Which version(s) does this affect? (Environment, OS, etc...)

release/2.0.0

[nazarhussain]

@4miners If we send the transactions in that particular order means (0.5, 0.5, 0.3), the block will always have transaction with the lowest balance first. That's what happens in the case above and clearly seen in the attached image as well.

The code responsible for it on 2.0.0 release is:

https://github.com/LiskHQ/lisk-sdk/blob/a8ad19b67677aa4abcfdcd28638319d7ca838644/framework/src/modules/chain/logic/block.js#L92-L97

This particular code is exactly the same in 1.6 release as well. So if same block forged on the 1.6 please will have same transaction order means lowest amount first.

https://github.com/LiskHQ/lisk-sdk/blob/40e2e86021ad14661dea78932d638a2a5bb53835/framework/src/modules/chain/logic/block.js#L89-L94

So for the case of forging block, I found consistent behaviour in 1.6 and 2.0, now looking further in particularly sync process if that order is kept intact in both releases.

[4miners]

Looks like the issue is with how we process the transactions. To replicate this I used rounds integration tests, as they contain useful tickAndValidate function. I added following scenario:

        let acc  = randomUtil.account();

        describe('credit account with 1 LSK', () => {
            const transactions = [];

            before(() => {
                const transaction = transfer({
                    recipientId: acc.address,
                    amount: '100000000',
                    passphrase: accountsFixtures.genesis.passphrase,
                });
                transactions.push(transaction);
            });

            tickAndValidate(transactions);
        });

        describe('credit account with 0.3 LSK, spend 0.5 LSK, spend 0.5 LSK', () => {
            const transactions = [];

            before(() => {
                const transaction1 = transfer({
                    recipientId: acc.address,
                    amount: '30000000',
                    passphrase: accountsFixtures.genesis.passphrase,
                });

                const transaction2 = transfer({
                    recipientId: randomUtil.account().address,
                    amount: '50000000',
                    passphrase: acc.passphrase,
                });

                const transaction3 = transfer({
                    recipientId: randomUtil.account().address,
                    amount: '50000000',
                    passphrase: acc.passphrase,
                });

                transactions.push(transaction1);
                transactions.push(transaction2);
                transactions.push(transaction3);
            });

            tickAndValidate(transactions);
        });

On version 2.0.0 those tests are passing, while on 1.6.0 they are failing on should contain all expected transactions. Forged block doesn't contain one 0.5 LSK spend transaction and order doesn't matter - it will never contain it and this test will always fail. However, in version 2.0.0 it will pass when we credit the account first (as in the tests above) but fail if we credit the account last (transaction2, transaction3, transaction1).

[4miners]

Going further, transactions in 1.6.0 are checked always against confirmed balance when they hit the transaction pool, prior beign selected to be included in a block.

The flow is following:
modules.transactions.processUnconfirmedTransaction->
transactionPool.processUnconfirmedTransaction ->
__private.processVerifyTransaction ->
logic.transaction.verify

There this check is executed:
https://github.com/LiskHQ/lisk-sdk/blob/40e2e86021ad14661dea78932d638a2a5bb53835/framework/src/modules/chain/logic/transaction.js#L676-L681

In the scenario described above those checks will pass and all transactions will be accepted by the transaction pool and go to queued queue. Then before forging a block transactionPool.fillPool is called.

The flow is following:
__private.applyUnconfirmedList ->
modules.transactions.applyUnconfirmed ->
logic.transaction.applyUnconfirmed

Then this check is executed:
https://github.com/LiskHQ/lisk-sdk/blob/40e2e86021ad14661dea78932d638a2a5bb53835/framework/src/modules/chain/logic/transaction.js#L1024-L1029

It fails on this check because unconfirmed balance of the account can be only be deducted (in case of sender), it's never credited (in case of recipient).

https://github.com/LiskHQ/lisk-sdk/blob/40e2e86021ad14661dea78932d638a2a5bb53835/framework/src/modules/chain/logic/transfer.js#L238-L240

For confimed transactions we credit recipient here:
https://github.com/LiskHQ/lisk-sdk/blob/40e2e86021ad14661dea78932d638a2a5bb53835/framework/src/modules/chain/logic/transfer.js#L174-L180

So it's not possible for 1.6.0 node to create a block that contains transactions which together spends more from sender account that its confirmed balance is at the time of block creation.

[nazarhussain]

Here is the detail analysis of the issue. Say we have account A which have balance of 1LSK. Now we send three transactions in this particular order.

1. A spend 0.5 LSK (0.1 fee)
2. A spend 0.5 LSK (0.1 fee)
3. A received 0.3 LSK

Transaction Pool
If we process this particular order of transactions, 1.6 will not include the 2nd transaction in the same block where we have transaction 1 and 3. The reason will be explained below.

Block Creation
If it pass through fillPool and create the block for three transactions, the order of transactions will be changed as below. Because while creating block we sort transactions by lower amount first.

1. A received 0.3 LSK
2. A spend 0.5 LSK (0.1 fee)
3. A spend 0.5 LSK (0.1 fee)

And this order is same for 1.6 and 2.0, so block created by both version for three transactions will be identical.

Block Processing
Now say we start processing the block which contains the transactions in above mentioned order. Here is the what happens in 1.6 release.
Process Block

1. Applied Uncofirmed for all transactions in block at once.
2. We deduct from sender unconfirmed balance, But we don't credit the recipient unconfirme balance
3. When 0.3 Credit was processed the recipient account didn't updated the unconfirmed balance and it was still 1LSK
4. When first 0.5 spend transaction process we dedicated the unconfirmed balance and it reaches 1 - 0.5 - 0.1 = 0.4LSK
5. While this process reaches to third transaction it failed, because account don't have the enough funds.

In 1.6 we are not doing any thing for applying unconfirmed transfer transaction, which is responsible to not credit unconfirmed balance of recipient.

https://github.com/LiskHQ/lisk-sdk/blob/40e2e86021ad14661dea78932d638a2a5bb53835/framework/src/modules/chain/logic/transfer.js#L238-L240

In 2.0 we don't have unconfirmed step and process transactions only in one way. The only difference is we don't persist the changes during verification step (similar to old unconfirmed behaviour).

Conclusion

1. I don't see any protocol definition which suggest to not do any thing during applying unconfirmed state
2. Having an empty applyUnconfirmed in 1.6 supposed to be an invalid approach to me. If we had unconfirmed balance we should have process it exactly the way we process confirms balance.
3. As per protocol definition the block seems valid, as if we follow the order of transaction in the block the balance of account never go negative.

Suggestion

1. To me its clearly a hardfork, as any such block created by 2.0 will never be processed by 1.6
2. I would suggest to announce 2.0 as hard fork and move forward
3. Other solution is to patch the transaction verification process in 2.0 to behave exactly like applyUnconfirmed before a certain height. And once we release any other hardfork in future we remove that condition.

@MaciejBaj @shuse2 @4miners @ManuGowda

[shuse2]

@nazarhussain
After discussing with @MaciejBaj , We will go with your suggestion 3!