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

multi: support config bitcoind fee estimate mode #4078

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

yyforyongyu
Copy link
Collaborator

This PR fixes issue #2990

Pull Request Checklist

  • If this is your first time contributing, we recommend you read the Code
    Contribution Guidelines
  • All changes are Go version 1.12 compliant
  • The code being submitted is commented according to Code Documentation and Commenting
  • For new code: Code is accompanied by tests which exercise both
    the positive and negative (error paths) conditions (if applicable)
  • For bug fixes: Code is accompanied by new tests which trigger
    the bug being fixed to prevent regressions
  • Any new logging statements use an appropriate subsystem and
    logging level
  • Code has been formatted with go fmt
  • Protobuf files (lnrpc/**/*.proto) have been formatted with
    make rpc-format and compiled with make rpc
  • For code and documentation: lines are wrapped at 80 characters
    (the tab character should be counted as 8 characters, not 4, as some IDEs do
    per default)
  • Running make check does not fail any tests
  • Running go vet does not report any issues
  • Running make lint does not report any new issues that did not
    already exist
  • All commits build properly and pass tests. Only in exceptional
    cases it can be justifiable to violate this condition. In that case, the
    reason should be stated in the commit message.
  • Commits have a logical structure according to Ideal Git Commit Structure

Copy link
Contributor

@wpaulino wpaulino left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, looks pretty good!

lnwallet/chainfee/estimator.go Outdated Show resolved Hide resolved
lnwallet/chainfee/estimator.go Outdated Show resolved Hide resolved
docs/INSTALL.md Outdated Show resolved Hide resolved
@Roasbeef Roasbeef requested a review from carlaKC March 24, 2020 00:58
Copy link
Collaborator

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Just two style nits from me, non-blocking :)

config.go Outdated Show resolved Hide resolved
config.go Show resolved Hide resolved
config.go Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
chainregistry.go Outdated Show resolved Hide resolved
@wpaulino
Copy link
Contributor

Alternatively, we could also extend any RPCs that already require fee-related parameters with the estimate mode instead of having the config option. Though I think the two can still go together, as the config option will be used as the default, and you can express your preference differently through the RPC if necessary. Extending the RPCs would be out of scope for this PR of course. Thoughts @yyforyongyu?

@yyforyongyu
Copy link
Collaborator Author

Alternatively, we could also extend any RPCs that already require fee-related parameters with the estimate mode instead of having the config option. Though I think the two can still go together, as the config option will be used as the default, and you can express your preference differently through the RPC if necessary. Extending the RPCs would be out of scope for this PR of course. Thoughts @yyforyongyu?

Since it would require the user to restart lnd if changing the estimate mode is needed, yes, it would be better if we could also provide the option in related RPCs. Atm, I believe there are target_conf and sat_per_byte, maybe a third fee_mode can be added?

However, I think it won't be done without a drawback. Specifically, fee_mode can only be applied to bitcoind backend. The good part about putting it in config is that we can specify it under bitcoin.bitcoind, the downside is it needs to be restarted to change the value. The chainregistry tried pretty hard to use chainControl as an abstraction so that we won't need to worry about the different backends, adding a fee_mode in RPCs would certainly break it a bit, I think.

@wpaulino
Copy link
Contributor

Specifically, fee_mode can only be applied to bitcoind backend. The good part about putting it in config is that we can specify it under bitcoin.bitcoind, the downside is it needs to be restarted to change the value.

Good point, we could return an error when using it with other backends. It's not something we need to worry about for this PR though.

@Roasbeef Roasbeef added this to the 0.10.0 milestone Mar 26, 2020
@Roasbeef Roasbeef merged commit 1c398d5 into lightningnetwork:master Mar 26, 2020
@yyforyongyu yyforyongyu deleted the bitcoind-estimatemode branch March 27, 2020 05:43
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