babylonchain / btc-staker

Other
24 stars 14 forks source link

Make staker command to use global parameters library #170

Closed KonradStaniec closed 2 months ago

KonradStaniec commented 2 months ago

Pr which make our commands use global params. It also do some cleanup.

This also fixes security audit concern BP1-008, which stated that user can create staking transaction which does not obey global params.

Notable changes:

RafilxTenfen commented 2 months ago

Could you update docs/create-phase1-staking.md instructions to use the global parameters? My bad, I read it wrong, the command create-phase1-staking-transaction still uses flags

and also what do you think about having an example of global parameters in the root folder as global-parameters-example.golden.json ?

RafilxTenfen commented 2 months ago

I tried to replicate the staking tx created with create-phase1-staking-transaction and create-phase1-staking-transaction-with-params

$ stakercli transaction create-phase1-staking-transaction --staker-pk 2dedbb66510d56b11f7a611e290f044e24dd48fd9c8a76d103ba05c8e95f3558 --finality-provider-pk a89e7caf57360bc8b791df72abc3fb6d2ddc0e06e171c9f17c4ea1299e677565 --staking-amount=5000000 --staking-time=52560 --magic-bytes=62627434 --covenant-committee-pks=50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0 --covenant-quorum=1 --network=signet

{
    "staking_tx_hex": "020000000002404b4c00000000002251204a4b057a9fa0510ccdce480fdac5a3cd12329993bac2517afb784a64d11fc1b40000000000000000496a4762627434002dedbb66510d56b11f7a611e290f044e24dd48fd9c8a76d103ba05c8e95f3558a89e7caf57360bc8b791df72abc3fb6d2ddc0e06e171c9f17c4ea1299e677565cd5000000000"
}

I have created a global parameter json as

{
  "versions": [
    {
      "version": 0,
      "activation_height": 198865,
      "staking_cap": 5000000000,
      "tag": "62627434",
      "covenant_pks": [
        "50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0"
      ],
      "covenant_quorum": 1,
      "unbonding_time": 1008,
      "unbonding_fee": 10000,
      "max_staking_amount": 5000000,
      "min_staking_amount": 50000,
      "max_staking_time": 64000,
      "min_staking_time": 64000,
      "confirmation_depth": 10
    }
  ]
}

And executed the params with

$ stakercli transaction create-phase1-staking-transaction-with-params ./global-params-example.json --staker-pk 2dedbb66510d56b11f7a611e290f044e24dd48fd9c8a76d103ba05c8e95f3558 --finality-provider-pk a89e7caf57360bc8b791df72abc3fb6d2ddc0e06e171c9f17c4ea1299e677565 --staking-amount=5000000 --staking-time=52560 --network=signet --tx-inclusion-height 198867

[btc-staker] error parsing file ./global-params-example.json: invalid params with version 0: invalid covenant public key 50929b74c1a04954b78b4b6035e97a5e078a5a0f28ec96d547bfee9ace803ac0: malformed public key: invalid length: 32

My question is, was the covenant-pk wrong in the first command create-phase1-staking-transaction ?

KonradStaniec commented 2 months ago

My bad, I read it wrong, the command create-phase1-staking-transaction still uses flags

and also what do you think about having an example of global parameters in the root folder as global-parameters->example.golden.json ?

Goot point about example, will add something.

Yup, I left create-phase1-staking-transaction as we need it for deposit, and deposit transactions will not obey parmaterters, also I did not want to break existing flow. I have added description to the command though, that it should be used only for some custom operations as it is less secure than command with parameters.

KonradStaniec commented 2 months ago

My question is, was the covenant-pk wrong in the first command create-phase1-staking-transaction ?

Key wasn't wrong, and in the scripts in both cases there will be valid key.

The difference is, in global params we require key to be 33byte representation, and in create-phase1-staking-transaction it must be in 32byte representation. We could change key format to be is 33 bytes represnation also in create-phase1-staking-transaction but this would require change in deposit transaction tooling so I am not sure it is worth it.