Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix decodeUnsignedTx to correctly try previous eras #2874

Merged

Conversation

paweljakubas
Copy link
Contributor

@paweljakubas paweljakubas commented Sep 7, 2021

Issue Number

ADP-1110

Comments

This PR adjusts decodeSignedTx so that when decoding in a given era, it also correctly tries all previous eras.

@paweljakubas paweljakubas self-assigned this Sep 7, 2021
@piotr-iohk
Copy link
Contributor

This doesn't seem to work. Actually sending HW device transaction crashes the wallet.
Cborized transaction produced by HW wallet from here -> https://input-output-rnd.slack.com/archives/GGKFXSKC6/p1630597099119600?thread_ts=1630579153.100300&cid=GGKFXSKC6

Sending it via wallet endpoint:

echo "83a40082825820d61b9b69bffa67975dc6bc51c3933d2f93cf1b78a000b0e22c762c6cad5229c30182582045c12d4910d70d073374ddd0c095f6a65ff10a3be72c0cb54680803d2d709f1a00018282583900290e0fc38ad639131f37acd052d71660e3f9bd29cf3af1c8f100f36e30aa53197bc7a67d43c26bc3cebb86ba1f0ceb2426ed89c9e2c556491a009896808258390019d628aaa31ad468084c7f34328f04bff60739d1d6cf93778c50e07ceacda39105223742da1efae9dfa82004b15b50a44efa6fea0c2c2dd51a02c1bbeb021a0002ac79031a0228e1eda100828258207de65747ddbbfbb6104a8aa0f825374bb234b726a09788f61c146bacd97b959558405871d80281d42506dde3bab0c0338fae04b08bb105116033432c70a9e4f18049fd8d7529e5a08e2a6d7bfbe91fd1e456bef7976a604e93d1cdb0165864c17b038258209b9f9d1bef02e831b70e66f5339800e8bfe6e5e7d635e181d6878335d83652c5584055f1a01f34bb99b9fc95fbb9d0f7f16566873edcd80d2c107ac35289d4bad12860203b9d349baaef023674005d1005ec15141b0e40a532fb5c32c294309bf20ff6" | xxd -r -p -> tx.bin

curl --header "Content-Type: application/octet-stream" \
-X POST \
--data-binary "@tx.bin" \
http://localhost:8090/v2/proxy/transactions

curl: (52) Empty reply from server

In the wallet log there is:

[cardano-wallet.main:Debug:4] [2021-09-07 15:05:45.60 UTC] Logging shutdown.
cardano-wallet: ExceptionInLinkedThread (ThreadId 14) unsafeSerializeCbor: DeserialiseFailure 431 "Size mismatch when decoding \nRecord RecD.\nExpected 3, but found 4."
CallStack (from HasCallStack):
  error, called at src/Cardano/Wallet/Unsafe.hs:163:12 in cardano-wallet-core-2021.8.27-7kvgZ5I8PBYLTDU4OW4Sil:Cardano.Wallet.Unsafe
  unsafeDeserialiseCbor, called at src/Cardano/Wallet/Shelley/Compatibility.hs:1199:7 in cardano-wallet-2021.8.27-6kBffRH7vq3GRVwBxOvC08:Cardano.Wallet.Shelley.Compatibility
  unsealShelleyTx, called at src/Cardano/Wallet/Shelley/Network.hs:496:41 in cardano-wallet-2021.8.27-6kBffRH7vq3GRVwBxOvC08:Cardano.Wallet.Shelley.Network
  withNetworkLayerBase, called at src/Cardano/Wallet/Shelley/Network.hs:309:5 in cardano-wallet-2021.8.27-6kBffRH7vq3GRVwBxOvC08:Cardano.Wallet.Shelley.Network
  withNetworkLayer, called at src/Cardano/Wallet/Shelley.hs:278:9 in cardano-wallet-2021.8.27-6kBffRH7vq3GRVwBxOvC08:Cardano.Wallet.Shelley


@paweljakubas paweljakubas force-pushed the paweljakubas/adp-1110/fix-proxy-to-accept-alonzo-tx branch from a722206 to b5118cd Compare September 7, 2021 19:16
@paweljakubas paweljakubas changed the title use applicative in decodeTx Bolster _decodeTx and unsealedTx Sep 7, 2021
@jonathanknowles
Copy link
Contributor

jonathanknowles commented Sep 8, 2021

I've done some manual testing of this branch with the following transaction submission command:

stack exec cardano-wallet -- transaction submit 83a40082825820d61b9b69bffa67975dc6bc51c3933d2f93cf1b78a000b0e22c762c6cad5229c30182582045c12d4910d70d073374ddd0c095f6a65ff10a3be72c0cb54680803d2d709f1a00018282583900290e0fc38ad639131f37acd052d71660e3f9bd29cf3af1c8f100f36e30aa53197bc7a67d43c26bc3cebb86ba1f0ceb2426ed89c9e2c556491a009896808258390019d628aaa31ad468084c7f34328f04bff60739d1d6cf93778c50e07ceacda39105223742da1efae9dfa82004b15b50a44efa6fea0c2c2dd51a02c1bbeb021a0002ac79031a0228e1eda100828258207de65747ddbbfbb6104a8aa0f825374bb234b726a09788f61c146bacd97b959558405871d80281d42506dde3bab0c0338fae04b08bb105116033432c70a9e4f18049fd8d7529e5a08e2a6d7bfbe91fd1e456bef7976a604e93d1cdb0165864c17b038258209b9f9d1bef02e831b70e66f5339800e8bfe6e5e7d635e181d6878335d83652c5584055f1a01f34bb99b9fc95fbb9d0f7f16566873edcd80d2c107ac35289d4bad12860203b9d349baaef023674005d1005ec15141b0e40a532fb5c32c294309bf20ff6

(The above is a serialized Mary-era transaction.)

On master, the above command produces the following error:

I couldn't verify that the payload has the correct binary format. Therefore I couldn't send it to the node. Please check the format and try again.

With this branch, we see the following error:

HardForkApplyTxErrFromEra S (S (S (S (Z (WrapApplyTxErr {unwrapApplyTxErr = ApplyTxError [UtxowFailure (WrappedShelleyEraFailure (UtxoFailure (ValueNotConservedUTxO (Value 0 (fromList [])) (Value 56426212 (fromList []))))),UtxowFailure (WrappedShelleyEraFailure (UtxoFailure (BadInputsUTxO (fromList [TxInCompact (TxId {_unTxId = SafeHash "45c12d4910d70d073374ddd0c095f6a65ff10a3be72c0cb54680803d2d709f1a"}) 0,TxInCompact (TxId {_unTxId = SafeHash "d61b9b69bffa67975dc6bc51c3933d2f93cf1b78a000b0e22c762c6cad5229c3"}) 1])))),UtxowFailure (WrappedShelleyEraFailure (UtxoFailure (OutsideValidityIntervalUTxO (ValidityInterval {invalidBefore = SNothing, invalidHereafter = SJust (SlotNo 36233709)}) (SlotNo 36705607))))]})))))

However, I believe this last error is expected, as this transaction has already been previously submitted to testnet. Therefore, it's attempting to spend already-spent UTxOs, which produces a failure, as expected.

Copy link
Contributor

@jonathanknowles jonathanknowles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reviewed the code, and it looks good.

I've made some minor changes to error strings (to make them more understandable), and removed a small amount of code duplication.

I've also confirmed that this branch produces the "expected" failure (see #2874 (comment)).

I haven't merged this yet, as I still need to be convinced that we've done a sufficient amount of testing.

@jonathanknowles
Copy link
Contributor

bors try

iohk-bors bot added a commit that referenced this pull request Sep 8, 2021
@jonathanknowles jonathanknowles changed the title Bolster _decodeTx and unsealedTx Fix decodeUnsignedTx to correctly try formats of previous era Sep 8, 2021
@jonathanknowles jonathanknowles changed the title Fix decodeUnsignedTx to correctly try formats of previous era Fix decodeUnsignedTx to correctly try previous eras Sep 8, 2021
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 8, 2021

try

Build succeeded:

@piotr-iohk
Copy link
Contributor

piotr-iohk commented Sep 8, 2021

Nice! I was able to produce serialized tx with Ledger Nano S and Daedalus and successfully send over:

curl --header "Content-Type: application/octet-stream" -X POST --data-binary "@tx.bin" http://localhost:8090/v2/proxy/transactions

{"id":"68400f96fa46cd4f1045583fcf84f3e9bcebe8d41d047893c2f4e9b18627da8a"}

(edit: confirmed also with Trezor and Ledger nano S on testnet and mainnet)

Copy link
Contributor

@piotr-iohk piotr-iohk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a good short term solution, thanks @paweljakubas, @jonathanknowles.

The test cases we are interested in are:

  1. wallet and node in alonzo era, transaction in {mary,allegra,shelley} era (current testnet).
  2. wallet and node in mary era, transaction in {mary,allegra,shelley} era (test cluster set to mary era, or mainnet).

As an indicator of successful decoding, transaction rejection by the node due to validation seems fine to me.

At present, it's not really feasible to test with a wide range of example transactions. For this, we should use signTx.

@rvl
Copy link
Contributor

rvl commented Sep 8, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Sep 8, 2021

Build succeeded:

@iohk-bors iohk-bors bot merged commit 9ae2d48 into master Sep 8, 2021
@iohk-bors iohk-bors bot deleted the paweljakubas/adp-1110/fix-proxy-to-accept-alonzo-tx branch September 8, 2021 07:38
rvl added a commit that referenced this pull request Sep 8, 2021
rvl added a commit that referenced this pull request Sep 8, 2021
rvl added a commit that referenced this pull request Sep 9, 2021
rvl added a commit that referenced this pull request Sep 9, 2021
rvl added a commit that referenced this pull request Sep 10, 2021
rvl added a commit that referenced this pull request Sep 14, 2021
rvl added a commit that referenced this pull request Sep 14, 2021
rvl added a commit that referenced this pull request Sep 14, 2021
rvl added a commit that referenced this pull request Sep 15, 2021
rvl added a commit that referenced this pull request Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants