Flaky BYRON_ADDRESSES, ADDRESS_IMPORT_05 - I can import 15000 of addresses

[Anviking]

Context

#2161 (comment)

The ADDRESS_IMPORT_05 - I can import 15000 of addresses test can probably be expected to be somewhat slow.

Test Case

https://github.com/input-output-hk/cardano-wallet/blob/4ad068a00187df54b52ba1c88ed68b2b264866bf/lib/core-integration/src/Test/Integration/Scenario/API/Byron/Addresses.hs#L401-L433

Failure / Counter-example

ci/hydra-build:required

 src/Test/Integration/Scenario/API/Byron/Addresses.hs:410:46:
 1) API Specifications, BYRON_ADDRESSES, ADDRESS_IMPORT_05 - I can import 15000 of addresses
 uncaught exception: IOException of type UserError
 user error (Waited longer than 90s (more than 2 epochs) for an action to resolve. Action: "Addresses are imported". Error condition: Just (HUnitFailure (Just (SrcLoc {srcLocPackage = "cardano-wallet-core-integration-2020.9.22-6DPlnrdhceJLXA8CsrMoUY", srcLocModule = "Test.Integration.Scenario.API.Byron.Addresses", srcLocFile = "src/Test/Integration/Scenario/API/Byron/Addresses.hs", srcLocStartLine = 430, srcLocStartCol = 13, srcLocEndLine = 430, srcLocEndCol = 35})) (ExpectedButGot Nothing "15000" "0")))

 To rerun use: --match "/API Specifications/BYRON_ADDRESSES/ADDRESS_IMPORT_05 - I can import 15000 of addresses/"

---

Resolution

---

QA

[Anviking]

The integration tests looks pretty simple, I'd guess the problem isn't there.

This should be the responsible function:

importRandomAddresses
    :: forall ctx s k.
        ( HasDBLayer s k ctx
        , RndStateLike s
        , k ~ ByronKey
        )
    => ctx
    -> WalletId
    -> [Address]
    -> ExceptT ErrImportRandomAddress IO ()
importRandomAddresses ctx wid addrs = db & \DBLayer{..} -> mapExceptT atomically $ do
    cp <- withExceptT ErrImportAddrNoSuchWallet
        $ withNoSuchWallet wid (readCheckpoint (PrimaryKey wid))
    let s0 = getState cp
        ours = scanl' (\s addr -> s >>= Rnd.importAddress addr) (Right s0) addrs
    case last ours of
        Left err ->
            throwE $ ErrImportAddr err
        Right s' ->
            withExceptT ErrImportAddrNoSuchWallet $
                putCheckpoint (PrimaryKey wid) (updateState s' cp)
  where
    db = ctx ^. dbLayer @s @k

There is an atomically.

I'm guessing though, that importRandomAddresses might inherently not work with rollbacks? The addresses are saved to a new checkpoint, which is thrown a way on rollback.

I'm guessing changing that would be pretty complex. And low priority, given that it is about legacy byron wallets. So we could mark it flaky?

[KtorZ]

@Anviking the point of the atomically is also to prevent rollbacks from affecting this function. As soon as importRandomAddresses is given access to the database, no rollback can happen until it has completed (because the restoration loop will pause).

The problem may not be in this function but somewhere else, a function that would read and update a checkpoint in two separate atomically, which would be really bad.

[Anviking]

no rollback can happen until it has completed (because the restoration loop will pause).

I know. But if importRandomAddresses merely inserts addresses to the latest checkpoint, then this doesn't matter. The rollback will wait, and then remove the checkpoint with the new addresses.

I'm thinking something like this:

Cps:   cp0   cp1   cp2   cp3
addrs: 0     0     0     0

>>>  importRandomAddresses

Cps:   cp0   cp1   cp2   cp3
addrs: 0     0     0     10

>>>  rollback to cp2

Cps:   cp0   cp1   cp2
addrs: 0     0     0 

>>>  apply blocks on new fork

Cps:   cp0   cp1   cp2   cp3
addrs: 0     0     0     0

[KtorZ]

Ouch. Yes. Because addresses are then added only at a recent slot which gets dropped with the rollback. That's quite bad and a pretty major issue.

[KtorZ]

Not flaky but a real issue: ADP-619