iotaledger / iota.go

IOTA Go API Library. Find documentation on https://wiki.iota.org/build/welcome
MIT License
357 stars 95 forks source link

Add and refactor FailureReason maps from iota-core #672

Closed muXxer closed 9 months ago

muXxer commented 9 months ago

Description

Rework the mapping from iotago errors to TransactionFailureReason.

The main motivation is to give better, more descriptive errors when it matters, which is important for the developer experience.

The approach here was to go through all errors in vm/vm.go and vm/nova/vm.go and check which errors actually can occur. Occasionally, we also call out into other parts of the codebase, and those were checked on a best-effort basis.

However, it is not necessarily desirable to map every single possible error case to a TransactionFailureReason, for the following reasons:

Some other errors that were previously mapped are no longer mapped:

Error level of detail

Another important point is that TransactionFailureReason is a simple byte, which brings along some challenges and trade-offs that need to be made. In that context, one interesting case are errors classes that can occur for multiple output types. For instance, there is ErrNewChainOutputHasNonZeroedID, for when a newly created chain output type has a non-zeroed chain ID. One could easily argue that there should be a dedicated error type for every chain output type: Account, Anchor, NFT, Foundry and Delegation Output. Since the TransactionFailureReason is just a byte, it's not possible to associate an error with an output type or even the concrete output index for which this error occured. In the absence of that, we need to make a choice whether to create dedicated error types for each case, e.g. ErrNewAccountOutputHasNonZeroedID, ErrNewAnchorOutputHasNonZeroedID, and so on, or if a single error is good enough.

The choice for this particular case is to use a single error. The rationale that could be applied here for figuring out, whether or not dedicated errors are needed, is whether debugging a transaction with such an error would be easy: Is it reasonably simple to find which chain output has the non-zeroed ID? Probably yes.

On the other hand, is InvalidStakingFeatureTransition easy enough to debug? Most likely not. So the more intricate the validation rules are, the more likely it is that dedicated errors are needed. Hence, why there are various specific error variants for Staking Feature, Block Issuer Feature and Delegation Output transition.

As a final example, there are also various cases in which a "CommitmentInputMissing" error is returned. However, since many different parts of a transaction could've caused the need for a Commitment Input, more specific errors are introduced, since a single one would be too unhelpful. Examples include:

One future improvement could be to make the failure reason not just a simple byte, but consist of multiple ones, where one indicates the error and another the offending output's index. Or one might even consider making such errors algebraic data types, where the failure reason can be associated with as much helpful information as is sensible.

Unlock errors

One area that could still see improvements is the input unlock errors. In this PR it was extended to have three different errors for chain address, direct unlockable and multi address errors. Judging by above's rationale, since the rules of how Signature Unlocks, Reference Unlocks and the chain-specific unlocks can be combined are fairly complex, we could consider making even better errors there.

Mapping the error

When we have an error like return ierrors.Join(iotago.ErrInvalidDirectUnlockableAddressUnlock, iotago.ErrUnlockSignatureInvalid, err), where we want both of those errors to be joined for an overall more helpful error message, and both are also part of the failure map, then we need to make a decision which one to map. The chosen approach is to map the more detailed error, since that should almost always be more helpful for debugging. In this case, iotago.ErrUnlockSignatureInvalid should be mapped. That means we cannot use the existing approach of iterating through the failure map and checking if any error is in the to-be-mapped error using ierrors.Is(...). Instead, we should get the error chain, and iterate it from back to front, so that we map the first, most-detailed error that is in the error chain. This is also much more efficient, since the previous approach (still used by the block failure determining code), runs through the entire error chain for every single error in the map, which is n*m time complexity. The new approach walks the error chain once and does len(error chain) number of lookups in the map. Since error chains are now effectively trees, we walk the tree in a post-order, depth-first traversal to collect the errors, to get the desired result.

VM Improvements

Some noteworthy things were also done as part of this PR:

Other Notes

"invalid unlock for multi address: invalid unlock for chain address 0x18bf8697430bff9afc755c3001eee52a2a261dfa881c641e35506bd1050cc390fe (AnchorAddress): input 2 is not unlocked through input 0's unlock"

before:

"input 1's address is already unlocked through input &{824639622464 0 map[]}'s unlock
but the input uses a non referential unlock: invalid input unlock"

after:

"invalid unlock for direct unlockable address:
input 1's address is already unlocked through input 0's unlock but the input
uses a non referential unlock of type SignatureUnlock"

before:

"input 1 has a chain address (*iotago.AccountAddress) but its corresponding
unlock is of type *iotago.ReferenceUnlock: invalid input unlock"

after:

"invalid unlock for chain address: input 1 has a chain address of type AccountAddress
but its corresponding unlock is of type ReferenceUnlock"
muXxer commented 9 months ago

LGTM, special thanks to @PhilippGackstatter for this massive PR. Even though I opened the PR, he did all the heavy lifting here :D