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

Adjust restoration loop to look for known certificate registrations #899

Closed
8 tasks done
KtorZ opened this issue Oct 23, 2019 · 7 comments
Closed
8 tasks done

Adjust restoration loop to look for known certificate registrations #899

KtorZ opened this issue Oct 23, 2019 · 7 comments
Assignees

Comments

@KtorZ
Copy link
Member

KtorZ commented Oct 23, 2019

Context

With #898, certificate registrations will now be part of blocks normally processed by the wallet layer.

Decision

  • Adjust restoration loop and keep track of certificates created by our wallet within already existing checkpoints.

Acceptance Criteria

  1. The wallet engine must keep track of all certificates (modulo checkpoints) discovered on the chain as long as their date (as slot) of activation
  2. The delegation status returned by the API must corresponds to the status maintained by the wallet engine (no certificate == not delegating, or, delegating according to the latest certificate).

Development plan

Generalization of existing discovery classes

PR: #1075

  • Generalize the definition of the IsOurs type class so that it can be applied to more types of entities than Address.
  • Add an IsOurs s ChimericAccount constraint to the Wallet type.

Adjust restoration loop to look for delegations from wallet's chimeric account

PR: #1050

  • Add implementation of IsOurs (SeqState n k) ChimericAccount.
  • Adjust the return type of applyBlock so that it references any stake delegation that has occurred on behalf of the specified wallet.
  • Adjust the definition of restoreBlocks so that it correctly processes the registrations returned by applyBlocks.
  • Fix atomiticity issue when inserting delegation certificates in a distinct transaction than checkpoints.

PRs

Number Base
#1075 master
#1050 master

QA

PR: #1050

  • Add tests to ensure that applyBlock does successfully discover delegations made by the wallet under test, and only that wallet. (STAKE_POOLS_JOIN_01)
  • Add tests to ensure that a wallet's delegation status is correctly updated in the context of discovered delegations. (STAKE_POOLS_JOIN_01)
@jonathanknowles
Copy link
Member

@KtorZ

Our current definition of Block in the wallet layer includes a set of delegations from ChimericAccount to PoolId:

data Block = Block
    { header  
        :: !BlockHeader   
    , transactions
        :: ![Tx]  
    , delegations 
        :: ![(ChimericAccount, PoolId)]   
    } deriving (Show, Eq, Ord, Generic)

The definition of applyBlock has access to a Wallet s, which implies an IsOurs s constraint, which in turns gives us the isOurs function:

class IsOurs s where  
    isOurs
        :: Address
        -> s  
        -> (Bool, s)  
        -- ^ Checks whether an address is ours or not. 

Of course, the above function gives us a way to recognize values of Address that are ours.

Question: how can we recognize values of ChimericAccount? Can values of ChimericAccount be treated as an Address (through unwrapping and rewrapping), or is there another function that we should be using here to translate?

@KtorZ
Copy link
Member Author

KtorZ commented Nov 25, 2019

Can values of ChimericAccount be treated as an Address (through unwrapping and rewrapping)

Definitely NO 😅

is there another function that we should be using here to translate?

We're going to need another function for that. Though, that function really is == :)
There's (by common agreement) a single chimeric account attached to a particular wallet. That chimeric account is known in advance so it's really a matter of finding it (or not) inside the list of certificates published in the current block or batch of blocks.

Additional reading here: https://github.com/input-output-hk/implementation-decisions/blob/e2d1bed5e617f0907bc5e12cf1c3f3302a4a7c42/text/1852-hd-chimeric.md

@jonathanknowles
Copy link
Member

@KtorZ wrote:

There's (by common agreement) a single chimeric account attached to a particular wallet. That chimeric account is known in advance so it's really a matter of finding it (or not) inside the list of certificates published in the current block or batch of blocks.

The applyBlock function is currently responsible for scanning a block and finding things that are relevant to the current wallet:

applyBlock
    :: Block
    -> Wallet s
    -> ([(Tx, TxMeta)], Wallet s)

Given that applyBlock only has a Block and Wallet s available to it, we'd need to update the wallet abstraction so that it's possible to check whether a given ChimericAccount is owned by a wallet.

Questions:

1. Do you have any preference as to how we adjust the Wallet abstraction?

2. Since there's only ChimericAccount attached to a wallet, do we even need to depend on the type variable s?

Could we use an oracle function similar to this:

data Wallet s where                                                                                                                                                                   
    Wallet :: (IsOurs s, NFData s, Show s)                                                                                                                                            
        => UTxO -- Unspent tx outputs belonging to this wallet                                                                                                                        
        -> BlockHeader -- Header of the latest applied block (current tip)                                                                                                            
        -> s -- Address discovery state
        -> IsOurChimericAccount                                                                                                                                               
        -> BlockchainParameters                                                                                                                                                       
        -> Wallet s

type IsOurChimericAccount = ChimericAccount -> Bool

iohk-bors bot added a commit that referenced this issue Nov 28, 2019
1075: Generalize `IsOurs` to work on different types of entity. r=jonathanknowles a=jonathanknowles

# Issue Number

#899 

# Overview

This PR:

- [x] Generalizes the definition of the `IsOurs` type class so that it can be applied to more types of entities than Address.
- [x] Adds an `IsOurs s ChimericAccount` constraint to the `Wallet` type.

# Comments

For the moment, we provide an instance of `IsOurs (SeqState n k) ChimericAccount` that always returns `False`. A further PR will add this implementation.

Co-authored-by: Jonathan Knowles <[email protected]>
iohk-bors bot added a commit that referenced this issue Nov 28, 2019
1050: Scan stake pool registrations during wallet restoration. r=KtorZ a=jonathanknowles

# Issue Number

#899 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

I have:

- [x] Provided an implementation of `IsOurs (SeqState n k) ChimericAccount`.
- [x] Adjusted the return type of `applyBlock` so that it references any stake delegation that has occurred on behalf of the specified wallet.
- [x] Adjusted the definition of `restoreBlocks` so that it correctly processes the registrations returned by `applyBlocks`.
- [x] Adjusted the definition of `applyBlock` to identify delegations relevant to the specified wallet, and return the final registration as part of `FilteredBlock`.
- [x] Added tests to ensure that `applyBlock` does successfully discover delegations made by the wallet under test, and only that wallet.
- [x]  Added tests to ensure that a wallet's delegation status is correctly updated in the context of discovered delegations.

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: KtorZ <[email protected]>
iohk-bors bot added a commit that referenced this issue Nov 28, 2019
1050: Scan stake pool registrations during wallet restoration. r=KtorZ a=jonathanknowles

# Issue Number

#899 

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

I have:

- [x] Provided an implementation of `IsOurs (SeqState n k) ChimericAccount`.
- [x] Adjusted the return type of `applyBlock` so that it references any stake delegation that has occurred on behalf of the specified wallet.
- [x] Adjusted the definition of `restoreBlocks` so that it correctly processes the registrations returned by `applyBlocks`.
- [x] Adjusted the definition of `applyBlock` to identify delegations relevant to the specified wallet, and return the final registration as part of `FilteredBlock`.
- [x] Added tests to ensure that `applyBlock` does successfully discover delegations made by the wallet under test, and only that wallet.
- [x]  Added tests to ensure that a wallet's delegation status is correctly updated in the context of discovered delegations.

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: Jonathan Knowles <[email protected]>
Co-authored-by: KtorZ <[email protected]>
@jonathanknowles
Copy link
Member

@KtorZ are you able to clarify the following:

Fix atomicity issue when inserting delegation certificates in a distinct transaction than checkpoints.

In particular, are you able to give a concrete set of reproduction steps for this issue? Thanks!

@KtorZ
Copy link
Member Author

KtorZ commented Dec 6, 2019

Ah! I forgot about this one, good I added a reminder ^.^ ... I'll explain in a PR, would be easier.

iohk-bors bot added a commit that referenced this issue Dec 6, 2019
1132: atomically write checkpoints, tx history and delegation certs when discovered r=KtorZ a=KtorZ

# Issue Number

<!-- Put here a reference to the issue this PR relates to and which requirements it tackles -->

#899

# Overview

<!-- Detail in a few bullet points the work accomplished in this PR -->

- [ ] I have adjusted the restoration loop to atomically write checkpoints, tx history and delegation certs when discovered

# Comments

<!-- Additional comments or screenshots to attach if any -->

<!-- 
Don't forget to:

 ✓ Self-review your changes to make sure nothing unexpected slipped through
 ✓ Assign yourself to the PR
 ✓ Assign one or several reviewer(s)
 ✓ Once created, link this PR to its corresponding ticket
 ✓ Assign the PR to a corresponding milestone
 ✓ Acknowledge any changes required to the Wiki
-->


Co-authored-by: KtorZ <[email protected]>
@jonathanknowles
Copy link
Member

Ah! I forgot about this one, good I added a reminder ^.^ ... I'll explain in a PR, would be easier.

Great! I've moved this to the QA state.

@piotr-iohk
Copy link
Contributor

lgtm.

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

No branches or pull requests

3 participants