Parse opreturn before other outputs #175
Conversation
Since the effects of opreturn act on the BSQ outputs it makes more sense to parse the opreturn before parsing the remaining outputs. Put the lock time in the LOCKUP txOutput. It makes more logical sense to keep it there.
| * OpReturn output might contain the lockTime in case of a LockTx. It has to be the last output. | ||
| * We store technically the lockTime there as is is stored in the OpReturn data but conceptually we want to provide | ||
| * it from the transaction. | ||
| * The locktime is stored in the LOCKUP txOutput, which is the first txOutput. |
There was a problem hiding this comment.
Hm... I am not sure if the first output is a good place here. Technically its the opReturn output, conceptually its the tx (thats why we use a delegate there).
There was a problem hiding this comment.
The reason I moved it to the LOCKUP txoutput is that elsewhere it's expected to reside there. It's also a logical place since that's the txoutput that has a restriction on it.
| @@ -154,6 +158,9 @@ public TxParser(PeriodService periodService, | |||
| DevEnv.logErrorAndThrowIfDevMode(msg); | |||
| } | |||
| } else { | |||
| // TODO(SQ): The transaction has already been parsed here and the individual txouputs are considered | |||
| // spendable or otherwise correct. Perhaps this check should be done earlier. | |||
|
|
|||
There was a problem hiding this comment.
I think you are correct. Should be probably done earlier
| txOutputParser.processOpReturnCandidate(outputs.get(lastIndex)); | ||
| int lastNonOpReturnIndex = lastIndex; | ||
| if (txOutputParser.processOpReturnCandidate(outputs.get(lastIndex))) { | ||
| txOutputParser.processTxOutput(true, outputs.get(lastIndex), lastIndex); |
There was a problem hiding this comment.
I am not 100% if that can cause problems. We can discuss tomorrow.
There was a problem hiding this comment.
From what I see there is only the lockup opreturn processing that really does anything here, and that was the one I wanted to fix. Would be good with more eyes to verify that of course.
| @@ -81,8 +82,9 @@ public void processGenesisTxOutput(TempTx genesisTx) { | |||
| } | |||
| } | |||
|
|
|||
| void processOpReturnCandidate(TempTxOutput txOutput) { | |||
| boolean processOpReturnCandidate(TempTxOutput txOutput) { | |||
There was a problem hiding this comment.
If we return a boolean we should rename to make that more clear (e.g. isOpReturnCandidate)
| @@ -164,6 +166,7 @@ private void handleBsqOutput(TempTxOutput txOutput, int index, long txOutputValu | |||
| optionalVoteRevealUnlockStakeOutput = Optional.of(txOutput); | |||
| } else if (isFirstOutput && opReturnTypeCandidate == OpReturnType.LOCKUP) { | |||
| bsqOutput = TxOutputType.LOCKUP; | |||
| txOutput.setLockTime(lockTime); | |||
There was a problem hiding this comment.
Why put it in first output? Technically its in opReturn, conceptually in Tx.
If a transaction is considered invalid it should consider all the txOutputs to be invalid. To avoid counting a txOutput as valid it's better to commit to stateservice only after checking the validity of the transaction.
|
I did some more refactoring, the main thing being that the txOutputs are only commited after the tx type is determined. That way we avoid inconsistency of adding some outputs to bsq stateservice and later finding out the tx is invalid. |
Since the effects of opreturn act on the BSQ outputs it makes more sense
to parse the opreturn before parsing the remaining outputs.
Put the lock time in the LOCKUP txOutput. It makes more logical sense to
keep it there.