ChainSafe / chainbridge-celo

GNU Lesser General Public License v3.0
3 stars 2 forks source link

Fetch Aggregated Signature from Istanbul block header #114

Closed waymobetta closed 3 years ago

waymobetta commented 3 years ago

Signature Verification parameters that are used to call the bridge smart contract method execute are malformed, i.e. the signature field is empty and therefore not being passed.

Description

Previously, the signature of the block that is to be proved was being fetched from the block header (blockData.EpochSnarkData().Signature) which was empty because according to the Celo team, the EpochSnarkData is only present in epoch blocks.

The aggregated signature, however, should be present in every block, and can be found within the extra data.

Related Issue Or Context

Closes: #102

How Has This Been Tested? Testing details.

Types of changes

Checklist:

github-actions[bot] commented 3 years ago

Go Test coverage is 40.8 %\ :sparkles: :sparkles: :sparkles:

github-actions[bot] commented 3 years ago

Go Test coverage is 40.8 %\ :sparkles: :sparkles: :sparkles:

github-actions[bot] commented 3 years ago

Go Test coverage is 40.8 %\ :sparkles: :sparkles: :sparkles:

github-actions[bot] commented 3 years ago

Go Test coverage is 40.7 %\ :sparkles: :sparkles: :sparkles:

github-actions[bot] commented 3 years ago

Go Test coverage is 40.7 %\ :sparkles: :sparkles: :sparkles:

github-actions[bot] commented 3 years ago

Go Test coverage is 40.8 %\ :sparkles: :sparkles: :sparkles:

waymobetta commented 3 years ago

This PR has been overly complicated for no reason- the fix was straightforward as we just import a utility function from Celo's celo-blockchain library to use for extracting the IstanbulExtra data from the block header.

The fix worked; the tests still did not pass.

Things got complicated when needing to utilize an interface in order to leverage gomock. Despite attemping to do so, the main issue- which was that the dummy block being used within the tests did not contain IstanbulExtra data- became lost in complexity behind needing to implement an interface, especially when the IstanbulExtra data was ignored anyway.

Example:

    extra, err := types.ExtractIstanbulExtra(block.Header())
    s.Nil(err)

    _ = utils.NewFungibleTransfer(
        listener.cfg.ID,
        destID,
        nonce,
        prop.ResourceID,
        &utils.MerkleProof{
            TxRootHash: block.TxHash(),
            Nodes:      proof,
            Key:        key,
        },
        &utils.SignatureVerification{
            AggregatePublicKey: pk,
            BlockHash:          block.Header().Hash(),
            Signature:          extra.AggregatedSeal.Signature,
        },
        prop.Amount,
        prop.DestinationRecipientAddress,
    )

A fix was pushed to create a new type of dummy block (replacing the existing) one that included InstanbulExtra data (borrowed from the celo-blockchain tests), which was then used to satisfy two checks that are performed on the extra field of the block, including:

  1. an IstanbulExtraVanity length check
  2. an RLP decode check on the raw data

After satisfying these requirements with the creation of the new dummyBlockWithIstanbulExtra, the tests passed.

github-actions[bot] commented 3 years ago

Go Test coverage is 40.8 %\ :sparkles: :sparkles: :sparkles:

github-actions[bot] commented 3 years ago

Go Test coverage is 40.8 %\ :sparkles: :sparkles: :sparkles:

P1sar commented 3 years ago

celo-blockchain tests

I beginning i mentioned that you should try build block header with valid Extra for tests not without reason before trying to use gomocks.

github-actions[bot] commented 3 years ago

Go Test coverage is 40.8 %\ :sparkles: :sparkles: :sparkles: