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.

Sign up for GitHub

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

Jump to bottom

fix: feed sataoshi/B to zetacore and check actual outTx size #1243

Merged
merged 9 commits into from
Oct 11, 2023
Merged

fix: feed sataoshi/B to zetacore and check actual outTx size #1243

merged 9 commits into from
Oct 11, 2023

Conversation

ws4charlie
Copy link
Contributor

@ws4charlie ws4charlie commented Oct 5, 2023 •
edited
Loading

Description

End user paid up to 3M satoshis ($800) on ZRC20 withdraw in mock mainnet for bitcoin outbound which is way more than enough. The correct gas price and gas limit needs to be used.

  1. Feed zetacore bitcoin gas price in unit of satoshis/Byte other than satoshis/KB because the gasLimit in ZRC20 contract is in unit of Byte instead of KB.
  2. Ensure actual outbound transaction size txSize falls in range [outTxBytesMin, outTxBytesCap].
  3. Use 4K as a more realistic (a statistical number based on historical outbound transactions) estimated size when selecting UTXOs. See google sheet link below
  4. The midian gasLimit is about 1600 Byte and the largest gasLimit is about 3200 Byte for ZRC20 configuration. See google sheet link below
  5. Please refer to historical sizes of bitcoin outTx

Closes: 1253

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Include instructions and any relevant details so others can reproduce.

  • Tested CCTX in localnet
  • Tested in development environment
  • Go unit tests
  • Go integration tests
  • Tested via GitHub Actions

Checklist:

  • I have added unit tests that prove my fix feature works

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

!!!WARNING!!!
nosec detected in the following files: zetaclient/bitcoin_client.go, zetaclient/btc_signer.go

Be very careful about using #nosec in code. It can be a quick way to suppress security warnings and move forward with development, it should be employed with caution. Suppressing warnings with #nosec can hide potentially serious vulnerabilities. Only use #nosec when you're absolutely certain that the security issue is either a false positive or has been mitigated in another way.

Only suppress a single rule (or a specific set of rules) within a section of code, while continuing to scan for other problems. To do this, you can list the rule(s) to be suppressed within the #nosec annotation, e.g: /* #nosec G401 */ or //#nosec G201 G202 G203
Broad #nosec annotations should be avoided, as they can hide other vulnerabilities. The CI will block you from merging this PR until you remove #nosec annotations that do not target specific rules.

Pay extra attention to the way #nosec is being used in the files listed above.

@github-actions github-actions bot added the nosec label Oct 5, 2023
@lumtis lumtis mentioned this pull request Oct 5, 2023
Closed
10 tasks
lumtis
lumtis reviewed Oct 5, 2023
View reviewed changes
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

We make the fix on the ZetaClient level but we still use the same value for the fee payment user-side:

gasPrice * GAS_LIMIT + PROTOCOL_FLAT_FEE

https://github.com/zeta-chain/protocol-contracts/blob/6005f2247c787ead18857f81717b844c086c9af4/contracts/zevm/ZRC20.sol#L245
When 3M satoshis is paid up on mock mainnet, is it the same value that has been calculated on the smart contract level?
We should ensure there are no discrepancies between the value paid on the smart contract and the actual fee paid by the TSS.

I believe we shouldn't need a change on the ZetaClient level, we should have the ability to control the fee value by modifying the parameters on the smart contract level: gasPrice, gasLimit

zetaclient/bitcoin_client.go Outdated Show resolved Hide resolved
@lumtis
Copy link
Member

lumtis commented Oct 6, 2023

@ws4charlie
We set the fee for the BTC tx here:

node/zetaclient/btc_signer.go

Line 97 in dec6172

fees := new(big.Int).Mul(big.NewInt(int64(tx.SerializeSize())), gasPrice)

The gas price is fetched from the gas price voted for the Bitcoin chain:
https://github.com/zeta-chain/node/blob/dec6172627009f37de6fd872d1837e4dc75ebfd4/x/crosschain/keeper/evm_hooks.go#L159C4-L159C4

Therefore if we can lower the gas price for the Bitcoin chain, I believe we can reduce equally the fees paid by the user on ZetaChain and the fees used by the TSS for the tx

lumtis
lumtis approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Checked deeper into the code. PostGasPrice will update the gas price value on the node, therefore no modification on the protocol side are required.

Overall, the fix looks good to me.

zetaclient/btc_signer.go Outdated Show resolved Hide resolved
@lumtis lumtis linked an issue Oct 6, 2023 that may be closed by this pull request
Fix feeRatePerByte value for Bitcoin PostGasPrice #1253
Closed
@lumtis
Copy link
Member

lumtis commented Oct 6, 2023

Just created an associated issue to track it for 10.1: #1253

@ws4charlie ws4charlie changed the title fix: feed sataoshi/B to zetacore and check size limit fix: feed sataoshi/B to zetacore and check actual outTx size Oct 8, 2023
@ws4charlie ws4charlie mentioned this pull request Oct 10, 2023
Closed
@ws4charlie ws4charlie merged commit f51f6e0 into develop Oct 11, 2023
10 of 11 checks passed
ws4charlie added a commit that referenced this pull request Oct 13, 2023
@ws4charlie @lumtis
@brewmaster012 @kevinssgh @kingpinXD
fix: merge develop into inbound-tracker and unified proof verification (
1ab1338 
#1292)

* refactor: change default mempool version in config (#1238)

* fix(`MsgWhitelistERC20`): set unique index for generate cctx (#1245)

* update index

* remove deprecated functions

* make generate

* add return value in message

* initialize test

* whitelist tests

* make generate

* fix(`observer`): remove error return in `IsAuthorized` (#1250)

* update function

* regenerate interfaces

* update for crosschain

* fix(`GetForeignCoinFromAsset`): Ethereum comparaison checksum/non-checksum format (#1261)

* fix error message

* compare with ETH address type

* tests

* goimport

---------

Co-authored-by: brewmaster012 <[email protected]>

* feat(`fungible`): add ability to update gas limit (#1260)

* add new field

* update message type

* message new logic

* fix: Blame index update (#1264)

* initial commit

* added queries and unit tests

* added cli

* fix parse error

* fix parse error 2

* fix lint and test errors

* ran make generate

* update index for keygen

* refactor query name

* refactor key calculation

* refactor lib name

* fix: feed sataoshi/B to zetacore and check actual outTx size (#1243)

* feed sataoshi/B to zetacore and check size limit

* removed fee cap

* replaced magic number 1000 with constant bytesPerKB

* put lowerbound, upperbound limit on sizeLimit

* use actual txSize for fee calculation

---------

Co-authored-by: charliec <[email protected]>

* fix: cherry pick all hotfix from v10.0.x (zero-amount, precision, etc.) (#1235)

* cherry pick all hotfix from v10.0.x

* adjusted code to for nosec

* adusted error handling and code comments according to PR review feedback

* cherry pick hotfix for bitcoin outbound performance and updated some log prints

* cherry pick mock mainnet hotfix for duplicate payment on nonce 0

---------

Co-authored-by: charliec <[email protected]>

* fix: register emissions grpc server (#1257)

* feat: Bitcoin block header and merkle proof (#1263)

* initiated bitcoin header and proof

* added smoke test for bitcoin merkle proof and RPC query

* make generate

* fix gosec and unit test

* Update common/headers_test.go

Co-authored-by: Lucas Bertrand <[email protected]>

* code adjustment according to feedback of PR review

* corrected a typo and added more comment to function

* fix gosec error

---------

Co-authored-by: charliec <[email protected]>
Co-authored-by: Lucas Bertrand <[email protected]>
Co-authored-by: brewmaster012 <[email protected]>

* fix: read gas limit from smart contract (#1277)

* read gas limit from smart contract

* add more checks for gas limit

* fix(`fungible`): add CLI command to query system contract (#1252)

* fix proto

* fix filename

* add cli query

* fix(`cmd`): add notice when using `--ledger` with Ethereum HD path (#1285)

* change comment

* add notice for ledger

* merge develop into inbound-tracker and unified proof verification

* fixed gosec errors

---------

Co-authored-by: Lucas Bertrand <[email protected]>
Co-authored-by: brewmaster012 <[email protected]>
Co-authored-by: kevinssgh <[email protected]>
Co-authored-by: charliec <[email protected]>
Co-authored-by: Tanmay <[email protected]>
@ws4charlie ws4charlie deleted the fix-btc-gasrate branch October 17, 2023 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinssgh kevinssgh kevinssgh approved these changes

@lumtis lumtis lumtis approved these changes

@brewmaster012 brewmaster012 Awaiting requested review from brewmaster012

@kingpinXD kingpinXD Awaiting requested review from kingpinXD

@fadeev fadeev Awaiting requested review from fadeev

Assignees
No one assigned
Labels
nosec
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix feeRatePerByte value for Bitcoin PostGasPrice
3 participants
@ws4charlie @lumtis @kevinssgh