-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
integration: optimize harness for better itest control, restore bitcoind compatibility #1659
Conversation
To allow using a custom btcd executable, we allow specifying a path to a file. If the path is empty, the harness will fall back to compiling one from scratch.
I just tested this with our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. Function overwrites looks a little odd, but I don't think there's a better way to do it.
Would love to get someone else to look at it before merging
Added a commit to fix this. |
The PR btcsuite#1594 introduced a change that made the order of parameters relevant, if one of them is nil. This makes it harder to be backward compatible with the same JSON message if an existing parameter in bitcoind was re-purposed to have a different meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🐲
// AllowHighFeesOrMaxFeeRate defines a type that can either be the legacy | ||
// allowhighfees boolean field or the new maxfeerate int field. | ||
type AllowHighFeesOrMaxFeeRate struct { | ||
Value interface{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, clever way to handle this!
Some improvements to the integration harness:
btcd
binary to be specified instead of always building it from sourceEverything falls back to the previous behavior if not otherwise specified. Existing users will just need to specify one more parameter.
EDIT: The PR now also contains a commit that restores compatibility with
bitcoind > 0.19
! There was a side effect of #1594 that caused thesendrawtransaction
command not to work anymore.@Roasbeef @wpaulino