consensus helper ExecTx ignoring errors from transaction batch execution

[muralisrini]

ExexTxs ignores error array from chaincode.ExecuteTransactions. This will result in transactions getting committed whether they succeeded or not.

[jyellick]

This is pending to be fixed by issue #579 and you can find additional information there.

@tuand27613 We can still address this with a stopgap solution, if you would like to implement it in helper.go, it should be a fairly straightforward fix, or feel free to assign to me, and I can quickly handle it.

This is all related to a discussion that occurred between myself, @srderson, @kchristidis, and @binhn. In short, ExecTxs used to return a []error from the transactions, and it was consensus' responsibility to remove transactions from the commit list which had encountered an error. There were two big problems with this, first, every consensus implementation must duplicate this logic, which increases the surface for bugs. Secondly, consensus should be agnostic to the details of transactions, it does not make sense that two consensus algorithms, who pick the same order for transactions, could result in two different blocks.

Finally, there are also multiple competing ideas as to whether failing transactions should be stored or not. Some use cases involve many many failing transactions, which could produce an excessively long blockchain with few valid transactions, and so wish to have invalid transactions excluded from the block. In other use cases, it may be very important that for auditing purposes all attempted transactions are recorded, regardless of their ability to be executed. This sort of complexity does not belong in consensus, and more likely belongs with the chaincode, or possibly some configuration.

@muralisrini Sorry for the verbose explanation, but I wasn't sure if you had any of the above on your radar.

[tuand27613]

@muralisrini , your thoughts ?

[jyellick]

@muralisrini Is this on your radar? We'd like to get this closed ASAP.

[muralisrini]

@jyellick @tuand27613 @binh

To capture the main point - ExecTxs has access to chaincode generated errors but ignores them today. (Summarizing @jyellick comment) this is because we do not know how to treat errors...we just save everything today.

Shoud we just add a "not started/succ/fail" enum to proto.Transaction ? ExecTx will set this flag to succ/fail

• Consensus can make use of this if it wants to
• it will be stored in the block (just an an additional int)... available for audit
• event clients have information about failed transaction too (SDK could provide additional mechanism to filter out succ or failed transactions from the block)
• minimal change (and we don't duplicate code by exposing errors to higher layers)

The above won't work if we think we shoud NEVER save a block with erred transactions in it.

In any case this is not a chaincode fix ... more of consensus/system to decide what to store on ledger.

@prjayach, fyi. Sounds like this is what you ran into in your application ?

[prjayach]

Yes, @muralisrini This will fix the issue. Once this is in, the events framework messages should include which transactions succeeded/failed in each block.

From an application point of view, I do want to know if a transaction failed -- either as part of the block or somewhere else where I can look it up.

[muralisrini]

Thanks, @prjayach .

@binhn, consensus, acutely sensitive that this is reopening discussions in this area...

Is adding "not started/succ/fail" field to proto.Transaction is right thing to do ? What this means

• a block would get committed if it passes consensus
• passing consensus just means everyone agrees on the block - does not mean all transactions in the block have succeeded
• note that this will not set the actual error itself

We have to look beyond current consensus mechanism to make sure this will work for future work as well.

[tuand27613]

linking to #752

[corecode]

This should be addressed by #1392 and can be closed.

[bmos299]

@muralisrini can you please close this one if it is fixed? Thanks, @bmos299

[tamasblummer]

@binh @muralisrini should not store transaction if chain code returns error. Reason: chain code can implement validation. Storing invalid transaction opens up the chain to severe DoS attacks or flood by incorrectly working peer. Please stay practical. Storing errors is not the job of the blockchain.

[bmos299]

@tamasblummer @muralisrini @binhn this sounds more like an architectural concern? What do you think?