Support for legacy UTxO witness in Jörmungandr #878

KtorZ · 2019-10-21T14:48:13Z

Issue Number

Overview

I have extended mkStdTx so that it would properly handle transactions coming from a RndKey and construct legacy-utxo witnesses for it.

Comments

Jörmungandr doesn't implement creating legacy utxo witnesses from jcli ...

jcli/src/jcli_app/transaction/mk_witness.rs

            WitnessType::OldUTxO => {
                // TODO unimplemented!()
                let _secret_key: SecretKey<Ed25519Bip32> = self.secret()?;
                Err(Error::MakeWitnessLegacyUtxoUnsupported)?;
                unimplemented!()
            }

so I had to construct them by hand according to:

The format defined in chain-impl-mockchain#witnesses
The format from the source (double checking it matches the doc..) chain-impl-mockchain/src/transaction/witness.rs

It'd be nice to review our golden if / when the constructor for the witness type gets updated.

KtorZ · 2019-10-21T14:52:37Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

-    putWitness (TxWitness bytes) = do
-        putWord8 1
-        putByteString bytes
+    putWitness (TxWitness bytes) = putByteString bytes


The tag is now part of the bytes themselves (cf utxoWitness and legacyUtxoWitness)

KtorZ · 2019-10-21T14:52:52Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

-        tag <- getTxWitnessTag
-        let len = txWitnessSize tag
+        tag <- lookAhead getTxWitnessTag
+        let len = txWitnessSize tag + txWitnessTagSize


The tag is now part of the bytes themselves (cf utxoWitness and legacyUtxoWitness)

KtorZ · 2019-10-21T14:53:34Z

lib/jormungandr/test/unit/Cardano/Wallet/Jormungandr/TransactionSpec.hs

@@ -190,13 +205,68 @@ mkStdTxSpec = do
            \080f5ac10474c7f6decbbc35f1620817b15bf1594981c034b7f6a15d0a24ec743d\
            \332f1774eb8733a5d40c"

-    describe "mkStdTx unknown input" $ do
-        unknownInputTest (Proxy @'Mainnet) block0
-        unknownInputTest (Proxy @'Testnet) block0


Moved down.

Anviking

👍 but barely looked at the tests

Anviking · 2019-10-21T16:01:05Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Transaction.hs

+    mkTxWitness (Hash block0) inps outs xprv =
+        let
+            xpub = getRawKey $ publicKey $ fst xprv
+            payload = block0 <> getHash (signData inps outs)


Nice!

Maybe a bit unfortunate that the payload-calculation is duplicated. Separate "signing" and "payload", or does things stop making sense then?

Now it would. Initially even the signing was different and I refactored a few things until the point where it got pretty similar. I ended up with this to also make testing a bit easier and the function signature slightly more readable than having a ByteString signature as argument.

Anviking · 2019-10-21T16:24:37Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Transaction.hs

@@ -109,13 +117,40 @@ newTransactionLayer (Hash block0) = TransactionLayer
            $ Left ErrExceededInpsOrOuts
    }

+-- | Provide a transaction witness for a given private key. The type of witness
+-- if different between types of keys and, with backward-compatible support, we
+-- may have to support many types for one backend target.


The type of witness if different between types of keys

The type of witness is different between different types of keys

Question: would it be possible to use a the random address scheme with new funds, and put sequential addresses in legacy funds (ignoring the specifics of the current mainnet and coming testnet)? If it is possible, maybe the "why" of the cited statement could be made a bit clearer.

we may have to support many types for one backend target

Why "may"? We are?

We are indeed.

would it be possible to use a the random address scheme with new funds

No, because the address structure for new funds makes it impossible to implement the random scheme.

put sequential addresses in legacy funds

No. Jörmungandr doesn't allow that.

extended comment to clarify that 👍

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Binary.hs

Anviking · 2019-10-21T16:38:41Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr.hs

-            rndApi <- apiLayer tr (toWLBlock <$> nl)
-            seqApi <- apiLayer tr (toWLBlock <$> nl)
+            let rndTl = newTransactionLayer @n (getGenesisBlockHash bp)
+            let seqTl = newTransactionLayer @n (getGenesisBlockHash bp)


~~Why do the transaction layers need to be different? newTransactionLayer is passed the same arguments.~~

Ah, right! The implicit key type-param!

KtorZ · 2019-10-22T09:55:02Z

bors r+

@disassembler

862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl Relates to #832 and #848. Supersedes #850. # Overview `cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). # Comments The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet. 864: nix: Provide derivations for Daedalus installer r=KtorZ a=rvl Relates to #863. Based on #828. # Overview - @disassembler @cleverca22 It's not exactly the same as before but should work ok I think. - Adds source filtering to avoid unnecessary rebuilds. # Comments To build: ``` nix-build -A cardano-wallet-jormungandr nix-build release.nix -A daedalus-jormungandr.windows -o daedalus-jormungandr-windows nix-build release.nix -A daedalus-jormungandr.macos -o daedalus-jormungandr-macos nix-build release.nix -A daedalus-jormungandr.linux -o daedalus-jormungandr-linux ``` Note that `daedalus-jormungandr.{windows,macos,linux}` from `release.nix` reference the same `cardano-wallet-jormungandr` derivation, only with different `system` or `crossSystem` arguments. So Daedalus may also import from `default.nix` rather than `release.nix`. 878: Support for legacy UTxO witness in Jörmungandr r=KtorZ a=KtorZ # Issue Number  #779 # Overview  - [x] I have extended `mkStdTx` so that it would properly handle transactions coming from a `RndKey` and construct `legacy-utxo` witnesses for it. # Comments  Jörmungandr doesn't implement creating legacy utxo witnesses from jcli ... [jcli/src/jcli_app/transaction/mk_witness.rs](https://github.com/input-output-hk/jormungandr/blob/master/jcli/src/jcli_app/transaction/mk_witness.rs#L78-L82) ```rust WitnessType::OldUTxO => { // TODO unimplemented!() let _secret_key: SecretKey<Ed25519Bip32> = self.secret()?; Err(Error::MakeWitnessLegacyUtxoUnsupported)?; unimplemented!() } ``` so I had to construct them by hand according to: - The format defined in [chain-impl-mockchain#witnesses](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/doc/format.md#witnesses) - The format from the source (double checking it matches the doc..) [chain-impl-mockchain/src/transaction/witness.rs](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/src/transaction/witness.rs#L173-L176) It'd be nice to review our golden if / when the constructor for the witness type gets updated.  879: Porting the rest of forget pending tx integration tests to CLI r=KtorZ a=paweljakubas # Issue Number  #836 # Overview  - [x] I have ported the rest of forgetting pending tx integration tests to CLI # Comments   Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com> Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>

iohk-bors · 2019-10-22T11:17:59Z

Canceled (will resume)

@disassembler

862: jormungandr launch: use --rest-listen instead of generating config.yaml r=KtorZ a=rvl Relates to #832 and #848. Supersedes #850. # Overview `cardano-wallet-jormungandr launch` specifies the REST API port and storage directory. The user provides the rest of the Jörmungandr configuration (e.g. trusted peers). # Comments The Jörmungandr P2P listen address, port, and log level are no longer configured by cardano-wallet. 864: nix: Provide derivations for Daedalus installer r=KtorZ a=rvl Relates to #863. Based on #828. # Overview - @disassembler @cleverca22 It's not exactly the same as before but should work ok I think. - Adds source filtering to avoid unnecessary rebuilds. # Comments To build: ``` nix-build -A cardano-wallet-jormungandr nix-build release.nix -A daedalus-jormungandr.windows -o daedalus-jormungandr-windows nix-build release.nix -A daedalus-jormungandr.macos -o daedalus-jormungandr-macos nix-build release.nix -A daedalus-jormungandr.linux -o daedalus-jormungandr-linux ``` Note that `daedalus-jormungandr.{windows,macos,linux}` from `release.nix` reference the same `cardano-wallet-jormungandr` derivation, only with different `system` or `crossSystem` arguments. So Daedalus may also import from `default.nix` rather than `release.nix`. 878: Support for legacy UTxO witness in Jörmungandr r=KtorZ a=KtorZ # Issue Number  #779 # Overview  - [x] I have extended `mkStdTx` so that it would properly handle transactions coming from a `RndKey` and construct `legacy-utxo` witnesses for it. # Comments  Jörmungandr doesn't implement creating legacy utxo witnesses from jcli ... [jcli/src/jcli_app/transaction/mk_witness.rs](https://github.com/input-output-hk/jormungandr/blob/master/jcli/src/jcli_app/transaction/mk_witness.rs#L78-L82) ```rust WitnessType::OldUTxO => { // TODO unimplemented!() let _secret_key: SecretKey<Ed25519Bip32> = self.secret()?; Err(Error::MakeWitnessLegacyUtxoUnsupported)?; unimplemented!() } ``` so I had to construct them by hand according to: - The format defined in [chain-impl-mockchain#witnesses](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/doc/format.md#witnesses) - The format from the source (double checking it matches the doc..) [chain-impl-mockchain/src/transaction/witness.rs](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/src/transaction/witness.rs#L173-L176) It'd be nice to review our golden if / when the constructor for the witness type gets updated.  879: Porting the rest of forget pending tx integration tests to CLI r=KtorZ a=paweljakubas # Issue Number  #836 # Overview  - [x] I have ported the rest of forgetting pending tx integration tests to CLI # Comments   Co-authored-by: Rodney Lorrimar <rodney.lorrimar@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com> Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>

iohk-bors · 2019-10-22T12:00:59Z

Build failed (retrying...)

buildkite/cardano-wallet

KtorZ · 2019-10-22T12:11:51Z

bors r-

iohk-bors · 2019-10-22T12:11:53Z

Canceled

KtorZ · 2019-10-22T15:14:34Z

bors r+

878: Support for legacy UTxO witness in Jörmungandr r=KtorZ a=KtorZ # Issue Number  #779 # Overview  - [x] I have extended `mkStdTx` so that it would properly handle transactions coming from a `RndKey` and construct `legacy-utxo` witnesses for it. # Comments  Jörmungandr doesn't implement creating legacy utxo witnesses from jcli ... [jcli/src/jcli_app/transaction/mk_witness.rs](https://github.com/input-output-hk/jormungandr/blob/master/jcli/src/jcli_app/transaction/mk_witness.rs#L78-L82) ```rust WitnessType::OldUTxO => { // TODO unimplemented!() let _secret_key: SecretKey<Ed25519Bip32> = self.secret()?; Err(Error::MakeWitnessLegacyUtxoUnsupported)?; unimplemented!() } ``` so I had to construct them by hand according to: - The format defined in [chain-impl-mockchain#witnesses](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/doc/format.md#witnesses) - The format from the source (double checking it matches the doc..) [chain-impl-mockchain/src/transaction/witness.rs](https://github.com/input-output-hk/chain-libs/blob/incentive/chain-impl-mockchain/src/transaction/witness.rs#L173-L176) It'd be nice to review our golden if / when the constructor for the witness type gets updated.  Co-authored-by: KtorZ <matthias.benkort@gmail.com>

iohk-bors · 2019-10-22T15:52:04Z

Build succeeded

buildkite/cardano-wallet

KtorZ requested a review from jonathanknowles October 21, 2019 14:48

KtorZ commented Oct 21, 2019

View reviewed changes

KtorZ self-assigned this Oct 21, 2019

KtorZ requested a review from Anviking October 21, 2019 14:56

Anviking approved these changes Oct 21, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/legacy-tx-witness branch from 273a98e to a2e7232 Compare October 22, 2019 09:54

KtorZ added 5 commits October 22, 2019 17:13

support legacy witness in Jormungandr transaction layer

8240593

Adjust Jormungandr Transaction spec to support multiple key schemes

89a23ca

add golden tests for legacy transaction signing

f708d2f

review comment for MkTxWitness & move shared parts outside of it

9fbf189

fix binary unit tests now that the witness includes its tag

6af04d7

KtorZ force-pushed the KtorZ/legacy-tx-witness branch from a2e7232 to 6af04d7 Compare October 22, 2019 15:13

iohk-bors bot merged commit 6af04d7 into master Oct 22, 2019

KtorZ deleted the KtorZ/legacy-tx-witness branch October 28, 2019 18:57

KtorZ added this to the Byron Wallet Support milestone Nov 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for legacy UTxO witness in Jörmungandr #878

Support for legacy UTxO witness in Jörmungandr #878

KtorZ commented Oct 21, 2019

KtorZ Oct 21, 2019

KtorZ Oct 21, 2019

KtorZ Oct 21, 2019

Anviking left a comment •

edited

Loading

Anviking Oct 21, 2019

KtorZ Oct 21, 2019

KtorZ Oct 21, 2019

Anviking Oct 21, 2019

KtorZ Oct 21, 2019 •

edited

Loading

KtorZ Oct 21, 2019

Anviking Oct 21, 2019

KtorZ commented Oct 22, 2019

iohk-bors bot commented Oct 22, 2019

iohk-bors bot commented Oct 22, 2019

KtorZ commented Oct 22, 2019

iohk-bors bot commented Oct 22, 2019

KtorZ commented Oct 22, 2019

iohk-bors bot commented Oct 22, 2019

Support for legacy UTxO witness in Jörmungandr #878

Support for legacy UTxO witness in Jörmungandr #878

Conversation

KtorZ commented Oct 21, 2019

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ Oct 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KtorZ commented Oct 22, 2019

iohk-bors bot commented Oct 22, 2019

Canceled (will resume)

iohk-bors bot commented Oct 22, 2019

Build failed (retrying...)

KtorZ commented Oct 22, 2019

iohk-bors bot commented Oct 22, 2019

Canceled

KtorZ commented Oct 22, 2019

iohk-bors bot commented Oct 22, 2019

Build succeeded

Anviking left a comment •

edited

Loading

KtorZ Oct 21, 2019 •

edited

Loading