Diverse witnesses in shelley #1849

paweljakubas · 2020-07-03T11:27:44Z

Issue Number

Overview

I have added WitnessTag
I have added impl of mkTx for Byron/Icarus wtinesses
I have added unit tests verifying decodeTx for Byron/Icarus witnesses
I have updated fee estimation
I have added integration tests illustrating the case

Comments

KtorZ · 2020-07-03T12:12:39Z

lib/shelley/test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs

+    let n = fromIntegral (natVal $ Proxy @(EntropySize mw)) `div` 8
+    bytes <- BS.pack <$> vector n
+    let ent = unsafeMkEntropy @(EntropySize mw) bytes
+    return $ entropyToMnemonic ent


Why not simply use genEntropy with EntropySize mw to get the right number of bytes?

KtorZ · 2020-07-03T12:13:58Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+        pure $ mkByronWitness pm sigData ks
+    pure
+        ( Tx (Hash sigData) (second coin <$> ownedIns) outs
+        , SealedTx $ CBOR.toStrictByteString $ CBOR.encodeSignedTx tx witnesses


I don't think this is right. This is how transaction were serialized in Byron, but in Shelley, they are following a different scheme. Therefore, we should serialize transaction using the new shelley methods; only the witnesses are different!

paweljakubas · 2020-07-08T17:21:03Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+                (k, pwd) <- lookupPrivateKey keyFrom addr
+                pure $ mkByronWitness unsigned pm (getRawKey k, pwd)
+            pure $ SL.WitnessSet mempty mempty bootstrapWits
+                <> mkExtraWits unsigned --- TO_DO - this is needed?


this one is maybe not needed - I do not know at this moment

Yes, if extra witnesses are needed for certificates or whatnot, they end up here

yeah, but in byron transactions it should not happen to my best knowledge, right?

KtorZ · 2020-07-09T08:31:47Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+          where
+            credential = Hash.UnsafeHash $ L8.toStrict chaff
+            addrAttr = Byron.mkAttributes $ Byron.AddrAttributes
+                       (Just $ Byron.HDAddressPayload (bloatChaff keyLen))


Why keyLen here 🤔 ? The HDAddressPayload is a ChaChaPoly digest + nonce that totals 28 bytes + 2 bytes because it's CBOR-encoded. I believe the keyLen is 32 bytes here so it's probably not a bit deal, but that is quite misleading and error-prone.

More importantly, there's only a HDPayload for /Byron/ type of addresses, Icarus do not.

addressed here 1209bca

KtorZ · 2020-07-09T08:32:45Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+            credential = Hash.UnsafeHash $ L8.toStrict chaff
+            addrAttr = Byron.mkAttributes $ Byron.AddrAttributes
+                       (Just $ Byron.HDAddressPayload (bloatChaff keyLen))
+                       (toByronNetworkMagic mainnetMagic)


mainnet is hard-coded here which is sort of wrong because mainnet addresses don't embed any discrimination attribute whereas testnet address do. So there's a significant size variation between the two.

suggestion: rely on the proxy above to know whether mainnet or testnet is needed. See also: fromNetworkDiscriminant

can I retrieve protocolMagic easily from proxy? 🤔
I have above (I agree it was quick hack) :

763 toByronNetworkMagic :: W.ProtocolMagic -> Byron.NetworkMagic 764 toByronNetworkMagic pm@(W.ProtocolMagic magic) = 765 if pm == W.mainnetMagic then 766 Byron.NetworkMainOrStage 767 else 768 Byron.NetworkTestnet (fromIntegral magic)

In theory they should match indeed.

KtorZ · 2020-07-09T08:39:12Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+    let signingKey = Crypto.SigningKey prv
+        bytes = xprvToBytes prv
+        addrAttr = Byron.mkAttributes $ Byron.AddrAttributes
+            (Just $ Byron.HDAddressPayload bytes)


I don't think we should send the user's private key over in a witness!
The HDAddressPayload corresponds to the encrypted derivation path that is present only on Byron-Random addresses. I assume that what we needs here is:

a) To pass down the corresponding address to mkByronWitness
b) Have a function in the Compatibility module to convert an Address into a AddrAttributes. We have all the information we need in the Address already.

have a try to deliver proper solution in 72c70d0

KtorZ

The witness for Byron needs to be corrected, in particular the HD payload needs to be inferred from the address!

Anviking · 2020-07-09T14:07:27Z

lib/shelley/src/Cardano/Wallet/Shelley/Transaction.hs

+                (k, pwd) <- lookupPrivateKey keyFrom addr
+                pure $ mkShelleyWitness unsigned (getRawKey k, pwd)
+
+            let withdrawalsWits


I was first wondering: why not factor out fmap Set.fromList $ forM (CS.inputs cs) $ \(_, TxOut addr _) from both cases, and just differentiate on the witness construction?

Then I see withdrawalsWits are only in the TxWitnessShelleyUTxO case. Wouldn't it technically be possible (at least for people using cardano-wallet as a library) to spend both byron funds and withdrawals? Would it not be as well to construct withdrawalsWits in both cases then?

that's good question - I have also this in the back of my head. I planned to have working byron/icarus tx...and then in the next PR check this possibility

to spend both byron funds and withdrawals?

Technically yes, but not with the wallet, because there are no withdrawals coming with our definition of Byron wallets.

…dresses

Doing to crypto takes time here, and the coin selection algorithm do estimate fee quite many times in the process. So instead, it's better to use dummy values.

The context fee estimator is showing its limit. It has actually become quite annoying to maintain two separate fee estimator. We should stop using this fee estimator and rely on the API instead. For now, I am manually adding margins necessary to account for Byron witnesses that are more expensive. Min and Max depends on whether we are making the transaction from an `Icarus` or `Random`. Fortunately, the API estimates this correctly!

paweljakubas · 2020-07-10T09:42:55Z

bors try

iohk-bors · 2020-07-10T10:33:22Z

try

Build succeeded

KtorZ · 2020-07-10T12:11:59Z

lib/core-integration/src/Test/Integration/Scenario/API/Shelley/Transactions.hs

+                (Link.getWallet @'Byron wByron) Default Empty
+            expectField
+                (#balance . #available)
+                (`shouldBe` Quantity (faucetAmt - feeEstMax - amt)) ra2


KtorZ

👍

paweljakubas · 2020-07-10T12:22:26Z

bors r+

1847: Track Stakepool Retirements in the DB r=jonathanknowles a=jonathanknowles # Related Issues #1815 #1816 #1817 #1819 #1880 # Overview This PR: - [x] Extends the pool DB schema to make it possible to store: - [x] retirement certificates. - [x] the _order_ of publication of certificates _within the same block_, for both registration and retirement certificates. This is necessary, as the intra-block order is _significant_.<br><br> - [x] Adds the `PoolLifeCycleStatus` type, which indicates the current lifecycle stage of a pool. There are currently _three_ possibilities: 1. `NotRegistered` (indicates that a pool has never been registered) 2. `Registered` (indicates that a pool has been registered but _not_ retired) (provides a registration certificate) 3. `RegisteredAndRetired` (indicates that a pool has been registered _and_ retired) (provides both a registration and a retirement certificate)<br><br> - [x] Adds the `readPoolLifeCycleStatus` function to the pool DB layer, making it possible to determine the current lifecycle status of a pool. - [x] Updates the `retirement` field of `ApiStakePool` with live information. # Property Tests This PR adds property tests to ensure that: - [x] the `determinePoolLifeCycleStatus` function respects the correct order of precedence for registration and retirement certificates: - for a given pool, a registration certificate always _supercedes_ a prior retirement certificate. - for a given pool, a retirement certificate always _augments_ the latest registration certificate. - [x] `readPoolLifeCycleStatus` queries work correctly in the presence of blocks containing multiple certificates for the same pool. - [x] database rollback works correctly in the presence of retirement certificates. # Future Work A **_future_** PR will add integration tests to ensure that: - retiring a pool eventually results in a correct update to the `retirement` field of `ApiStakePool`, when read through the `ListStakePools` operation. - retiring and then re-registering a pool clears the `retirement` field. See QA section of #1819. 1849: Diverse witnesses in shelley r=paweljakubas a=paweljakubas # Issue Number  #1844 # Overview  - [x] I have added WitnessTag - [x] I have added impl of mkTx for Byron/Icarus wtinesses - [x] I have added unit tests verifying decodeTx for Byron/Icarus witnesses - [x] I have updated fee estimation - [x] I have added integration tests illustrating the case # Comments   1887: Rename our SlotNo to SlotInEpoch r=Anviking a=Anviking # Issue Number #1868  # Overview  - [x] Renamed `SlotNo` to `SlotInEpoch` (for consistency with other components) # Comments - `W.SlotNo` == `Cardano.SlotInEpoch` /= `Cardano.SlotNo` ``` Epoch 0 1 2 3 4 SlotNo* 0 1 2 3 4 5 6 7 8 9 SlotInEpoch 0 1 0 1 0 1 0 1 0 1 ``` *) I.e. the one defined in cardano-base and used in ourobouros-consensus.   Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: IOHK <[email protected]> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Johannes Lund <[email protected]>

iohk-bors · 2020-07-10T12:34:25Z

Build failed (retrying...)

ci/hydra-eval

1849: Diverse witnesses in shelley r=paweljakubas a=paweljakubas # Issue Number  #1844 # Overview  - [x] I have added WitnessTag - [x] I have added impl of mkTx for Byron/Icarus wtinesses - [x] I have added unit tests verifying decodeTx for Byron/Icarus witnesses - [x] I have updated fee estimation - [x] I have added integration tests illustrating the case # Comments   1887: Rename our SlotNo to SlotInEpoch r=Anviking a=Anviking # Issue Number #1868  # Overview  - [x] Renamed `SlotNo` to `SlotInEpoch` (for consistency with other components) # Comments - `W.SlotNo` == `Cardano.SlotInEpoch` /= `Cardano.SlotNo` ``` Epoch 0 1 2 3 4 SlotNo* 0 1 2 3 4 5 6 7 8 9 SlotInEpoch 0 1 0 1 0 1 0 1 0 1 ``` *) I.e. the one defined in cardano-base and used in ourobouros-consensus.   Co-authored-by: Pawel Jakubas <[email protected]> Co-authored-by: IOHK <[email protected]> Co-authored-by: KtorZ <[email protected]> Co-authored-by: Johannes Lund <[email protected]>

iohk-bors · 2020-07-10T14:44:31Z

Build failed (retrying...)

ci/hydra-build:required

iohk-bors · 2020-07-10T16:14:37Z

Build succeeded

paweljakubas added this to the (ADP-357) Create byron/icarus txs in shelley milestone Jul 3, 2020

paweljakubas self-assigned this Jul 3, 2020

paweljakubas added the ADDING FEATURE Mark a PR as adding a new feature, for auto-generated CHANGELOG label Jul 3, 2020

KtorZ reviewed Jul 3, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/1844/diverse-witnesses-in-shelley branch 4 times, most recently from 6c90fca to 919f096 Compare July 8, 2020 17:17

paweljakubas commented Jul 8, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/1844/diverse-witnesses-in-shelley branch from 919f096 to 2d46562 Compare July 8, 2020 19:31

KtorZ reviewed Jul 9, 2020

View reviewed changes

KtorZ suggested changes Jul 9, 2020

View reviewed changes

paweljakubas force-pushed the paweljakubas/1844/diverse-witnesses-in-shelley branch from 2d46562 to 1ef9f16 Compare July 9, 2020 10:15

Anviking reviewed Jul 9, 2020

View reviewed changes

paweljakubas and others added 13 commits July 10, 2020 11:41

add tx witness tag

2978a22

mkWitness -> mkShelleyWitness

af3c578

introduce mkByronWitness

ebfe587

specialize witness creation in mkTx depending on witness tag

34d1f4c

add test in TransactionSpec for byron witness

aae2494

Regenerate nix

2a9e2f2

sneaking witness tag into _minimumFee and _estimateMaximumNumberOfInputs

af140bb

dependant computeTxSize on witness size

bcd7c5c

make Byron witness rountrip in TransactionSpec working

0f6f7cc

make shelley integration main compile

e7d8475

add integration test with byron witnesses

2405328

add toHDAddressPayload

61cbcb0

correct partially dummy values

3370b50

KtorZ and others added 8 commits July 10, 2020 11:41

add Byron & Icarus faucet wallets to Shelley genesis

d02d3b4

sign Byron transaction with unencrypted private keys!

ea17c78

make sure to unwrap the nested HDPayload when deconstructing Byron ad…

d292723

…dresses

do not create actual Byron witness when estimating

c49c69a

Doing to crypto takes time here, and the coin selection algorithm do estimate fee quite many times in the process. So instead, it's better to use dummy values.

remove feeEstimator and use fee endpoint

d22c284

address protocolMagic in computeTxSize

076a866

add tests for both testnet and mainnet

28a2f9f

paweljakubas force-pushed the paweljakubas/1844/diverse-witnesses-in-shelley branch from ac35bb6 to 28a2f9f Compare July 10, 2020 09:41

paweljakubas marked this pull request as ready for review July 10, 2020 09:42

iohk-bors bot added a commit that referenced this pull request Jul 10, 2020

Try #1849:

ae08613

paweljakubas requested a review from KtorZ July 10, 2020 09:43

KtorZ reviewed Jul 10, 2020

View reviewed changes

KtorZ approved these changes Jul 10, 2020

View reviewed changes

test Icarus wallet

3abc351

iohk-bors bot merged commit f6e6180 into master Jul 10, 2020

iohk-bors bot deleted the paweljakubas/1844/diverse-witnesses-in-shelley branch July 10, 2020 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Diverse witnesses in shelley #1849

Diverse witnesses in shelley #1849

paweljakubas commented Jul 3, 2020 •

edited

Loading

KtorZ Jul 3, 2020

KtorZ Jul 3, 2020

paweljakubas Jul 8, 2020

Anviking Jul 8, 2020

paweljakubas Jul 9, 2020

KtorZ Jul 9, 2020

paweljakubas Jul 9, 2020

KtorZ Jul 9, 2020

KtorZ Jul 9, 2020

paweljakubas Jul 9, 2020

KtorZ Jul 9, 2020

KtorZ Jul 9, 2020

paweljakubas Jul 9, 2020

KtorZ left a comment

Anviking Jul 9, 2020 •

edited

Loading

paweljakubas Jul 9, 2020

KtorZ Jul 10, 2020

paweljakubas commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

KtorZ Jul 10, 2020

KtorZ left a comment

paweljakubas commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

Diverse witnesses in shelley #1849

Diverse witnesses in shelley #1849

Conversation

paweljakubas commented Jul 3, 2020 • edited Loading

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

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

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 left a comment

Choose a reason for hiding this comment

Anviking Jul 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

try

Build succeeded

Choose a reason for hiding this comment

KtorZ left a comment

Choose a reason for hiding this comment

paweljakubas commented Jul 10, 2020

iohk-bors bot commented Jul 10, 2020

Build failed (retrying...)

iohk-bors bot commented Jul 10, 2020

Build failed (retrying...)

iohk-bors bot commented Jul 10, 2020

Build succeeded

paweljakubas commented Jul 3, 2020 •

edited

Loading

Anviking Jul 9, 2020 •

edited

Loading