search

dmigwi / sample

0 stars 0 forks source link

Rebroadcast unmined tx should only submit the specific pending tx #524

Open dmigwi opened 1 year ago

dmigwi commented 1 year ago

Currently when rebroadcasting a specific pending tx, all txs within mempool are rebroadcasted making the functionality feel unresponsive especially when there are many pending txs.

image

(Issue originally authored by: @Kennedy Izuegbu in gitlab.com)

dmigwi commented 1 year ago

i believe if there a several unmined tx, no one would want to having any tx unmined, so a republish once approach is desirable

moreover i believe while implementing that feature, the logic was handled in a gorutine, so it should be non blocking

By Kennedy Izuegbu on 2022-11-18T10:58:21 (imported from GitLab)

(Comment originally authored by: @Kennedy Izuegbu in gitlab.com)

dmigwi commented 1 year ago

Despite it being implementated in a goroutine on the UI end, each pending tx has to be submitted one a time. This is after querying all the unmined instead of just submitting the specific one needed by the user.

rebroadcasted making the functionality feel unresponsive

The context of this is when a user has to wait for: All the unmined txs to be retrieved and each an every one of those txs submitted for there to be a successful republishing modal shown on the UI. The more pending txs exist the more unresponsive the functionality will look because it will take longer to respond to the user.

By Migwi Ndung'u on 2022-11-18T11:02:19 (imported from GitLab)

(Comment originally authored by: @Kennedy Izuegbu in gitlab.com)

dmigwi commented 1 year ago

regardless of the number of unmined tx, the feature is not instant(from my tests on unmined dcr transactions) one thing we could do is hide the button and display a loader when the rebroadcast is in progress.

how many subsequent pending tx is going to exist in a wallet at any given time. it's also not a feature that a user needs urgent feedback on, hence the loader suggestion.

also the upstream dcr method publishes all unmined tx once the method is called asset.Internal().DCR.PublishUnminedTransactions, i don't see the need to have dcr behave one way and then btc another way for the same functionality.

if we must go forward with this, it shouldn't be now though

By Kennedy Izuegbu on 2022-11-18T11:14:28 (imported from GitLab)

(Comment originally authored by: @Kennedy Izuegbu in gitlab.com)

dmigwi commented 1 year ago

also the upstream dcr method publishes all unmined tx once the method is called asset.Internal().DCR.PublishUnminedTransactions, i don't see the need to have dcr behave one way and then btc another way for the same functionality.

I looked up the code and its possible to rebroadcast one tx by using the peer parameter passed to asset.Internal().DCR.PublishUnminedTransactions. As shown here: https://github.com/decred/dcrwallet/blob/v2.0.3/wallet/wallet.go#L5102-L5116.

On BTC end, each pending Tx must be submitted one at a time: https://pkg.go.dev/github.com/btcsuite/btcwallet@v0.12.0/wallet#Wallet.PublishTransaction

if we must go forward with this, it shouldn't be now though

True, this isn't a top priority in getting an MVP. We don't expect to have many pending txs in the app, although when they start showing up they may become overwhelming

By Migwi Ndung'u on 2022-11-18T11:26:33 (imported from GitLab)

(Comment originally authored by: @Kennedy Izuegbu in gitlab.com)