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

Adopt economyFee recommendation from mempool rate provider. #6239

Merged
merged 1 commit into from Jun 14, 2022
Merged

Adopt economyFee recommendation from mempool rate provider. #6239

merged 1 commit into from Jun 14, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jun 6, 2022

From bisq-network/proposals#343 (comment)

image

image

Jun-06 10:46:25.182 [Timer-17] INFO  b.p.m.p.MempoolFeeRateProvider$Fourth: Retrieved estimated mining fee of 10 sat/vB and economyFee of 1 sat/vB from mempool.bisq.services 
Jun-06 10:46:25.182 [Timer-17] INFO  b.p.m.p.MempoolFeeRateProvider$Fourth: refresh took 247 ms. 
Jun-06 10:46:25.390 [Timer-18] INFO  b.p.m.p.MempoolFeeRateProvider$First: Retrieved estimated mining fee of 10 sat/vB and economyFee of 2 sat/vB from mempool.space 
Jun-06 10:46:25.390 [Timer-18] INFO  b.p.m.p.MempoolFeeRateProvider$First: refresh took 454 ms. 
Jun-06 10:46:25.653 [Timer-20] INFO  b.p.m.p.MempoolFeeRateProvider$Second: Retrieved estimated mining fee of 10 sat/vB and economyFee of 1 sat/vB from mempool.emzy.de 
Jun-06 10:46:25.653 [Timer-20] INFO  b.p.m.p.MempoolFeeRateProvider$Second: refresh took 717 ms. 
Jun-06 10:46:25.662 [Timer-19] INFO  b.p.m.p.MempoolFeeRateProvider$Third: Retrieved estimated mining fee of 10 sat/vB and economyFee of 2 sat/vB from mempool.ninja 
Jun-06 10:46:25.662 [Timer-19] INFO  b.p.m.p.MempoolFeeRateProvider$Third: refresh took 726 ms. 

@wiz
Copy link
Contributor

wiz commented Jun 6, 2022

@jmacxx thanks for making this PR so quickly. however, are we sure that all the bugs in bisq's fee calculation have been fixed so that the 1 sat/vByte fee withdrawal doesn't cause rounding errors down to 0.999 sat/vByte as it did a few years ago - will need to test this inside bisq :)

@wiz
Copy link
Contributor

wiz commented Jun 7, 2022

another thing is that we will need to ship mempool v2.4 and have emzy and devin upgrade their mempool instances before we can merge this PR

@wiz
Copy link
Contributor

wiz commented Jun 9, 2022

here is the original 1 sat/vByte issue for context, I believe it was fixed when SegWit was added into Bitcoin but we need to test #3306

@wiz
Copy link
Contributor

wiz commented Jun 9, 2022

ok @Emzy @devinbileck can you please upgrade to mempool v2.4.0 so we can proceed with testing this PR
https://github.com/mempool/mempool/releases/tag/v2.4.0

@Emzy
Copy link
Contributor

Emzy commented Jun 9, 2022

ok @Emzy @devinbileck can you please upgrade to mempool v2.4.0 so we can proceed with testing this PR https://github.com/mempool/mempool/releases/tag/v2.4.0

Done.

@devinbileck
Copy link
Member

mempool.bisq.services has been upgraded to 2.4.0

@wiz
Copy link
Contributor

wiz commented Jun 10, 2022

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK

@ripcurlx ripcurlx added this to the v1.9.2 milestone Jun 14, 2022
@ripcurlx ripcurlx merged commit dd4be1c into bisq-network:master Jun 14, 2022
@ghost ghost mentioned this pull request Jun 28, 2022
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.

4 participants