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

feat(btc|service): support internal query data caching in TxBuilder.payFee() #184

Merged
merged 4 commits into from
May 24, 2024

Conversation

ShookLyngs
Copy link
Collaborator

@ShookLyngs ShookLyngs commented May 23, 2024

Changes

Query data cache

Previously, when collecting inputs for paying fees, the BtcAssetsApi.getBtcUtxos() API and the BtcAssetsApi.getRgbppAssetsByBtcUtxo() API were queried multiple times. In this PR, a DataCache is added to prevent querying the same API with the same params too often.

Reference: #173 (comment)

Test

@ShookLyngs
Copy link
Collaborator Author

ShookLyngs commented May 23, 2024

Here are some suggestions from @ahonn:

  • No need to expose the noUtxosCache option in the TxBuilder.payFee() (to the users)
  • The noUtxosCache option can be set to true by default when paying fee

@Dawn-githup
Copy link
Collaborator

Dawn-githup commented May 24, 2024

https://pudge.explorer.nervos.org/xudt/0xdc50aa08391b9f7831e08a70884e1094dae8984412e52a8c8371ca3bbccc44f5

Verify that there are 36 cells:

  • develop branch build transaction time::
image
  • This branch builds transaction time:
image

@duanyytop
Copy link
Collaborator

https://pudge.explorer.nervos.org/xudt/0xdc50aa08391b9f7831e08a70884e1094dae8984412e52a8c8371ca3bbccc44f5

Verify that there are 36 cells:

  • develop branch build transaction time::
image * This branch builds transaction time: image

Is there still room for improvement in the time cost reduction by the cache? Or is this test case not typical enough?

@ShookLyngs
Copy link
Collaborator Author

Just renamed the cache-related options to make them look clearer:

  • noUtxosCache -> noAssetsApiCache
  • cacheKey -> internalCacheKey

@ShookLyngs
Copy link
Collaborator Author

Is there still room for improvement in the time cost reduction by the cache? Or is this test case not typical enough?

The problem can be broken down into two parts:

  1. The test case is using the sendRgbppUtxos() API for testing, which is indeed not typical enough because this PR aims to improve the construction time for transactions that collect a large amount of inputs during the fee-paying process.
  2. The sendRgbppUtxos() API can be optimized individually, as it contains time-costly syntax like for (...) { await fetch() }, which can be refactored using the await Promise.all() API.

@Flouse Flouse merged commit 98469c5 into develop May 24, 2024
1 check passed
@Flouse Flouse deleted the feat/query-data-cache branch May 30, 2024 07:15
@Flouse Flouse linked an issue May 30, 2024 that may be closed by this pull request
2 tasks
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

Successfully merging this pull request may close these issues.

Building BTC TX with an address having excessive UTXOs takes forever
5 participants