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

Bump cardano-node from 1.32.1 to 1.33.0 #3071

Merged
merged 24 commits into from
Jan 11, 2022

Conversation

rvl
Copy link
Contributor

@rvl rvl commented Jan 4, 2022

Updates cardano-node version to 1.33.0.

Comments

Unblocks #2781.

Issue Number

ADP-1315

@rvl rvl self-assigned this Jan 4, 2022
@rvl rvl force-pushed the rvl/adp-1315/cardano-node-1.33.0 branch 2 times, most recently from 61925d4 to c971bce Compare January 5, 2022 07:40
@Anviking
Copy link
Member

Anviking commented Jan 5, 2022

bors try

iohk-bors bot added a commit that referenced this pull request Jan 5, 2022
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 5, 2022

try

Build failed:

@Anviking
Copy link
Member

Anviking commented Jan 5, 2022

Interesting:

  test/unit/Cardano/Wallet/Shelley/TransactionSpec.hs:380:29:
  1) Cardano.Wallet.Shelley.Transaction, Sign transaction, signTransaction adds reward account witness when necessary (AnyCardanoEra AlonzoEra)
       uncaught exception: ArithException
       arithmetic underflow
       (after 85 tests and 4 shrinks)
         ("\128\129\207\155\211\140\193\241\187\223\b\DC3|\CAN$\192\248\231t^\a\SOH\160\151\166\DC3\139\236\ETB'G\247\146\158\"\185\222\238>> 2f5\153\US\232\&9q\ESC\151T\aQ\244\&7\195'\fc\198b\255G\241\227\140\159b\141\204\191\175\150)\207\ACKR|R\215\ENQ?;)K\226\NUL\228\198\STXci\188\157\189",Passphrase <scrubbed-bytes>)
         UTxO {unUTxO = fromList [(TxIn {inputId = Hash "\249\247w v\141\137\\QR>\183\222\237\176\143\245\215\192\222\146\222\149\NAK\146\156\225Q\225\228\DC4,", inputIx = 1},TxOut {address = Address "\SOH\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL\NUL", tokens = TokenBundle {coin = Coin 31, tokens = TokenMap (fromList [(UnsafeTokenPolicyId (Hash "\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136\136"),NonEmptyMap {least = (UnsafeTokenName "TokenF",TokenQuantity 4), rest = fromList []})])}})]}
         Coin 1
         ShelleyTxBody ShelleyBasedEraMary (TxBodyConstr TxBodyRaw {_inputs = fromList [TxInCompact (TxId {_unTxId = SafeHash "01f4b788593d4f70de2a45c2e1e87088bfbdfa29577ae1b62aba60e095e3ab53"}) 661089,TxInCompact (TxId {_unTxId =

[...]

All similar properties fail for the AlonzoEra version specifically.

Would think it's due to the node bump rather than my changes to assignScriptRedeemers, but not 100% sure.

Make-shift debugging with mapException suggests it originates from the (==) of TxBodyContent 🤔

@sevanspowell
Copy link
Contributor

Still scratching my head on this one. I've almost ruled out the Eq instance for TxBodyContent, I don't think that's the issue.

@sevanspowell
Copy link
Contributor

Maybe it's just because Eq forces evaluation.

@sevanspowell
Copy link
Contributor

I created a fork/branch where I tried to binary-search the fields of TxBodyContent for the issue. The issue still exists if the instance is simply _ == _ = True, so I'm almost certain the issue isn't with the Eq instance.

https://github.com/sevanspowell/cardano-node/tree/sevanspowell/debugging

The Eq instance does inspect the fields of the TxBodyContent though, so it's probably forcing evaluation of something bad.

@sevanspowell
Copy link
Contributor

I fixed a small (unrelated) issue with the StakePoolMetadata generator though, because it was preventing me from debugging with larger size values.

@sevanspowell
Copy link
Contributor

I've got to leave this for today. For added confusion:

prop_signTransaction_neverChangesTxBody (AnyCardanoEra era) rootK utxo extraKeys =
    forAll (genTxInEra era) $ \tx -> do
        let
            tl = testTxLayer

            sealedTx = traceOnException "sealedTxFromCardano'" $
                sealedTxFromCardano' tx
            sealedTx' =  traceOnException "signTransaction"
                signTransaction tl (AnyCardanoEra era) (lookupFnFromKeys extraKeys) (first liftRawKey rootK) utxo sealedTx

            -- txBodyContent :: InAnyCardanoEra Cardano.Tx -> InAnyCardanoEra (Cardano.TxBodyContent Cardano.ViewTx)
            -- txBodyContent :: InAnyCardanoEra Cardano.Tx -> InAnyCardanoEra (TxIns Cardano.ViewTx)
            -- txBodyContent (InAnyCardanoEra e (Cardano.Tx (Cardano.TxBody bodyContent) _wits)) =
            --     InAnyCardanoEra e (bodyContent)

            -- bodyContentBefore = traceOnException "before" $ txBodyContent $ cardanoTx sealedTx
            -- bodyContentAfter = traceOnException "after" $ txBodyContent $ cardanoTx sealedTx'

        -- nothing traced:
        -- (traceOnException "before" bodyContentBefore) == (traceOnException "after" bodyContentAfter)

        -- compare is traced -- the exception seems to originate from the (==)
        --
        traceOnException "sealedTx" $ serialisedTx sealedTx' == serialisedTx sealedTx'
  where
    traceOnException n = mapException (\(e :: SomeException) -> userError $ n <> ": " <> show e )

This throws the same arithmetic underflow error...

@sevanspowell
Copy link
Contributor

Wait, so the problem might be in signTransaction/the generator.

Is this the offending code?

fromCardanoTxIn
    :: Cardano.TxIn
    -> W.TxIn
fromCardanoTxIn (Cardano.TxIn txid (Cardano.TxIx ix)) =
    W.TxIn (fromShelleyTxId $ Cardano.toShelleyTxId txid) (fromIntegral ix)

if the ix generated was negative, this would underflow.

@sevanspowell
Copy link
Contributor

Hmm, maybe not...

@sevanspowell
Copy link
Contributor

Actually I could be right, I'm seeing two sets of suspicious negative numbers in the Tx output from the test:

  1. In the collateral:
 _collateral = fromList [TxInCompact (TxId {_unTxId = SafeHash "01f4b788593d4f70de2a45c2e1e87088bfbdfa29577ae1b62aba60e095e3ab53"}) (-9003518531505017806)
  1. and in the Script:
Constr (-12)

That "Data" value is supposed to pick a constructor, I don't think negative numbers make sense in that context.

But likely the issue is #1.

@sevanspowell
Copy link
Contributor

Current theory crafting:

  1. Generator generates large word value for Cardano.Api.TxIx, for a Cardano.Api.Tx:
genTxIndex :: Gen TxIx
genTxIndex = do
    (Large (n :: Word)) <- arbitrary
    pure $ TxIx n

Cardano.Api.TxIx is a Word, so no problem here:

newtype TxIx = TxIx Word
  1. Cardano.Api.TxIn gets converted to a Cardano.Ledger.TxIn:
data TxIn crypto = TxInCompact !(TxId crypto) {-# UNPACK #-} !Int

Cardano.Ledger.TxIn index is an Int, so Word values larger than the maxBound :: Int overflow to a negative number.

  1. In the implementation of signTransaction, we call fromCardanoTxIn, which converts the ledger TxIn into our wallet type:
data TxIn = TxIn
    { inputId
        :: !(Hash "Tx")
    , inputIx
        :: !Word32
    } deriving (Read, Show, Generic, Eq, Ord)

Our wallet TxIx is Word32, so negative integers cause an arithmetic underflow when fromIntegral is called in our conversion:

fromCardanoTxIn
    :: Cardano.TxIn
    -> W.TxIn
fromCardanoTxIn (Cardano.TxIn txid (Cardano.TxIx ix)) =
    W.TxIn (fromShelleyTxId $ Cardano.toShelleyTxId txid) (fromIntegral ix)

Fun times.

@sevanspowell
Copy link
Contributor

Aha!

Fixed previous failures, but further failures in the Byron era:

         Exception thrown while showing test case:
           Transaction input index is too big, acceptable value is up to 2^32-1, in input TxIn "01f4b788593d4f70de2a45c2e1e87088bfbdfa29577ae1b62aba60e095e3ab53" (TxIx 5890982618161823609)
           CallStack (from HasCallStack):
             error, called at src/Cardano/Api/Gen.hs:1266:17 in cardano-wallet-core-2021.12.15-6YyEmhcEPYY4vmKRUHoLWN:Cardano.Api.Gen
         

  To rerun use: --match "/Cardano.Wallet.Shelley.Transaction/Sign transaction/signTransaction never changes tx body (AnyCardanoEra ByronEra)/"

Upon further investigation, it seems Cardano.Api correctly handles TxIx overflows in the Byron era but not the Shelley era:

makeByronTransactionBody :: TxBodyContent BuildTx ByronEra
                         -> Either TxBodyError (TxBody ByronEra)
makeByronTransactionBody TxBodyContent { txIns, txOuts } = do
    ins' <- NonEmpty.nonEmpty (map fst txIns) ?! TxBodyEmptyTxIns
    for_ ins' $ \txin@(TxIn _ (TxIx txix)) ->
      guard (txix <= maxByronTxInIx) ?! TxBodyInIxOverflow txin
makeShelleyTransactionBody era@ShelleyBasedEraAlonzo
...
          (Set.fromList (map (toShelleyTxIn . fst) txIns))

-- toShelleyTxIn returns no errors

@sevanspowell
Copy link
Contributor

Fixed a separate issue in simplifyAddress:

  • simplifyAddress simplifies generated addresses to a trivial address.
  • The underlying Ledger parsing function for addresses has changed to fail if it
    doesn't consume all input.
    • This revealed a bug in our current simplifyAddress implementation, where too
      many bytes were added to the end of pointer addresses.
  • Fix implementation of simplifyAddress so that test passes.
  • Improve error reporting in test.

@sevanspowell
Copy link
Contributor

I think the only remaining failures are the serialization failures:

       Serialization has changed. Compare golden file with test/data/Cardano/Wallet/Api/ApiCredential.faulty.reencoded.json.

My brain is kind of fried now, so not sure I'll figure them out. I've not dealt with these kinds of errors before, didn't they used to automatically override the old files?

@sevanspowell
Copy link
Contributor

Speculative PR for adding TxIndex overflow errors to cardano-api: IntersectMBO/cardano-node#3478

@sevanspowell
Copy link
Contributor

I've given fixing the golden JSON files a go, hopefully that works.

rvl and others added 17 commits January 7, 2022 16:49
- There is a limit of 512 bytes to StakePoolMetadata, constrain generator to
  generate values within this limit (roughly).
- Restrict TxIndex generator to generate values within the range [0, maxBound ::
Int]. This is because the underlying ledger type is:

data TxIn crypto = TxInCompact !(TxId crypto) {-# UNPACK #-} !Int

So if we generate values larger than the maxBound of an Int, we get overflow
errors.
- In the Byron era, maximum value for a TxIx is 2 ^ 32 - 1.
- simplifyAddress simplifies generated addresses to a trivial address.
- The underlying Ledger parsing function for addresses has changed to fail if it
  doesn't consume all input.
  - This revealed a bug in our current simplifyAddress implementation, where too
    many bytes were added to the end of pointer addresses.
- Fix implementation of simplifyAddress so that test passes.
- Improve error reporting in test.
- Tweak naming
- Provide more details to users
@Anviking Anviking force-pushed the rvl/adp-1315/cardano-node-1.33.0 branch from fca237d to 852adff Compare January 7, 2022 21:41
@Anviking
Copy link
Member

Anviking commented Jan 7, 2022

bors try

iohk-bors bot added a commit that referenced this pull request Jan 7, 2022
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 7, 2022

try

Build succeeded:

@@ -652,3 +671,68 @@ selectRndStatePending wid = do
where
assocFromEntity (RndStatePendingAddress _ accIx addrIx addr) =
((W.Index accIx, W.Index addrIx), addr)

{-------------------------------------------------------------------------------
Provide ReaderT instance for MonadSTM
Copy link
Member

Choose a reason for hiding this comment

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

Looks like a MonadSTM instance for ReaderT, though, no?

@piotr-iohk
Copy link
Contributor

bors r+

iohk-bors bot added a commit that referenced this pull request Jan 11, 2022
3071: Bump cardano-node from 1.32.1 to 1.33.0 r=piotr-iohk a=rvl

Updates cardano-node version to [1.33.0](https://github.com/input-output-hk/cardano-node/releases/tag/1.33.0).

- [x] Update cardano-node and libs in `stack.yaml`
- [x] Update cardano-node and libs in `cabal.project`
- [x] Bump Stackage LTS to latest version.
- [x] Fix build failures due to cardano-api / cardano-ledger changes.
- [x] Fix build failures due to `io-classes` change in IntersectMBO/ouroboros-network#3512.
- [x] Update version matrix in README.md

### Comments

Unblocks #2781.

### Issue Number

ADP-1315

Co-authored-by: Rodney Lorrimar <[email protected]>
Co-authored-by: Johannes Lund <[email protected]>
Co-authored-by: Samuel Evans-Powell <[email protected]>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 11, 2022

Build failed:

    signTransaction adds extra key witness when necessary (AnyCardanoEra MaryEra)
      +++ OK, passed 100 tests (100% feature not supported in MaryEra.).
    signTransaction adds extra key witness when necessary (AnyCardanoEra AlonzoEra) (75337ms)
      +++ OK, passed 100 tests.
    signTransaction adds tx in witnesses when necessary (AnyCardanoEra ByronEra) (1ms)
      +++ OK, passed 100 tests.

Unit test timeout on linux.

#2472

@Anviking
Copy link
Member

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Jan 11, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 25ef435 into master Jan 11, 2022
@iohk-bors iohk-bors bot deleted the rvl/adp-1315/cardano-node-1.33.0 branch January 11, 2022 12:52
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