Switch to more accurate fee estimation endpoint

[cd2357]

The API endpoint for fee estimations has been changed to one that delivers more accurate fee estimations.

This is a temporary solution, until a more decentralized approach is found.

Fixes projects/issues/27

[cbeams]

You beat me to it @cd2357; excellent :)

Reviewing now, thanks.

[cbeams]

I am in the middle of this review, but cannot finish right now. From what I've seen so far, everything looks in order. There are a few small tweaks I'm making, but nothing fundamental. @bisq-network/pricenode-operators, could one or more of you build from this PR and take it for a spin on your side?

Kudos, @cd2357 for the quick fix, and seriously great that you implemented the test ;)

[wiz]

Thanks for implementing this. Can you make it configurable with an environment variable or command line option, and leave the default as mempool.space? This way pricenode operators can optionally set it to their own mempool backend if they have one

[cbeams]

Can you make it configurable with an environment variable or command line option, and leave the default as mempool.space

Let's do that in a second PR. I have another change that I'd like to see if @cd2357 wants to make as part of my review.

Let's get this out as is and asap. My review is incoming in one minute.

[wiz]

Actually, I think pricenode code allows for multiple data providers, can you also add https://mempool.emzy.de/ as a second provider and poll from both of them?

[wiz]

Let's do that in a second PR

Okay :)

[cbeams]

ACK. @cd2357, please see the review commit that I just pushed.

Note that there is an opportunity here for one or more follow-on PRs as well.

The first is to eliminate the maxBlocks field from BitcoinFeeRateProvider. You'll see my comments to this effect. Rolling this out means changing the startup scripts and communicating effectively about the change to @bisq-network/pricenode-operators. It would be great if you want to do this, @cd2357.

The second is implementing the configurability that @wiz mentioned in the comments on this PR. We could discuss the best way to do that. It would also be good to deliberate a little further on the implications of querying multiple backends first, though. We've never been in a situation where we could expect to get very divergent results for fee rate estimations, because we've always all been calling the same (earn.com) endpoint. We shouldn't be too hasty on making that switch, which is part of the reason I want to ship this fix as is, nice and minimal.

Really great work here, @cd2357. Thanks!

@wiz, when we merge this, will you drive the @bisq-network/pricenode-operators upgrading?

[wiz]

Well if we merge this as-is and made all pricenode operators upgrade, we would make all of Bisq depend on my single node for mempool fees... can you add emzy's node in as a secondary source so they both get polled? I believe the code already supports this, right?

[Emzy]

FYI my bitcoinaverage subscription will run out 06/06

[sqrrm]

@wiz @cbeams Do you consider this merge ready or do you want to add emzy's node for slightly less centralization first?

[wiz]

@Emzy don't worry, we can allocate additional budget if necessary, this mempool thing is high priority

@sqrrm chris is right, this is kinda urgent, let's merge this PR for now and add emzy in a separate PR later

[cbeams]

Good to go from my side.

…

On May 5, 2020, at 6:33 PM, wiz ***@***.***> wrote:  @wiz approved this pull request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[cbeams]

@cd2357, a couple notes on commit e18f040:

• Per Follow the seven rules laid out in "How to Write a Git Commit Message" style#9, please wrap commit message bodies at 72 chars.
• The Fixes projects/issues/27 you included in the message body used incorrect issue reference syntax; it should have been Migrate Bisq pricenodes from earn.com fee estimation API to our own self-hosted mempool fee estimation API service projects#27. More importantly, though, this PR does not actually "fix" or "resolve" that issue. That issue is a larger project proposal, and would have stayed open regardless whether this PR were merged or not. In such cases, it's better to avoid the special closing keywords and simply write See bisq-network/projects#27 or This is a stopgap measure related to bisq-network/projects#27, etc, so that the links still get established.

[cd2357]

Really great work here, @cd2357. Thanks!

Thanks, glad I could help :)

Note that there is an opportunity here for one or more follow-on PRs as well.

The first is to eliminate the maxBlocks field from BitcoinFeeRateProvider. You'll see my comments to this effect. Rolling this out means changing the startup scripts and communicating effectively about the change to @bisq-network/pricenode-operators. It would be great if you want to do this, @cd2357.

If I understood that correctly, this PR should cover it: #4240. I assume there are startup or service scripts which are not committed, which the operators have on their systems. If so, that's one thing they'll have to manually adjust (already described that in the PR comment for their reference).

As to communicating it them, I'm not sure who whey are so I can tag them, nor can I use the @bisq-network/pricenode-operators tag you used @cbeams (it doesn't resolve for me, it just stays as text). So I'd leave the comm part to someone else, perhaps @wiz ?

The second is implementing the configurability that @wiz mentioned in the comments on this PR.

Already started with that, a PR is coming soon.

[cd2357]

@cd2357, a couple notes on commit e18f040 (...)

Cool, thanks for the hints. Wasn't aware GitHub can do all that, good to know.