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

An RPC connection fix and some vbyte fun #3772

Merged
merged 3 commits into from
Jun 18, 2020

Conversation

darosior
Copy link
Collaborator

@darosior darosior commented Jun 16, 2020

90bce56: I regularly hit this corner case on a bloated-memory node

9230907 and 2fbd490 fix #3771.

Fixes #3771

…ution

Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
@darosior darosior requested a review from cdecker as a code owner June 16, 2020 12:14
@cdecker
Copy link
Member

cdecker commented Jun 16, 2020

ACK 2fbd490

Copy link
Collaborator

@ZmnSCPxj ZmnSCPxj left a comment

Choose a reason for hiding this comment

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

Also, second commit should probably reword to use #3771 instead.

@@ -474,7 +474,7 @@ u32 feerate_from_style(u32 feerate, enum feerate_style style)
case FEERATE_PER_KBYTE:
/* Everyone uses satoshi per kbyte, but we use satoshi per ksipa
* (don't round down to zero though)! */
return (feerate + 3) / 4;
return feerate / 4 + 3;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this lead to us computing a higher feerate than "Everyone" later on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hesitated to use a "return FEERATE_FLOOR if < FEERATE_FLOOR ", but I think we were actually computing lower feerate than bitcoind before this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay. Will ACK this. Unless you want to change the second commit message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will need to fixup the tests, so will amend the second commit message while at it.

@ZmnSCPxj
Copy link
Collaborator

ACK 2fbd490

@darosior
Copy link
Collaborator Author

This messed up all feerate-related tests : now I regret not to have made the API in WU for estimatefees...

@darosior
Copy link
Collaborator Author

darosior commented Jun 17, 2020

Ok, so I went for the "don't go below FEERATE_FLOOR approach instead of monkey-patching all the hardcoded feerates in the tests (note that we do use feerate_from_style() for more "important" code, but we already check for floor puncture here).

We passed below the floor when the user specified `1000perkb`.
Matt Whitlock says :

    I was withdrawing with feerate=1000perkb, which should be the minimum-allowed fee rate. Indeed, bitcoin-cli getmempoolinfo reports:

    {
      "loaded": true,
      "size": 15097,
      "bytes": 9207924,
      "usage": 32831760,
      "maxmempool": 64000000,
      "mempoolminfee": 0.00001000,
      "minrelaytxfee": 0.00001000
    }

Changelog-fixed: rpc: The `feerate` parameters now correctly handle the standardness minimum when passed as `perkb`.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
Reported-by: Matt Whitlock
Copy link
Collaborator

@niftynei niftynei left a comment

Choose a reason for hiding this comment

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

ACK e473129

@ZmnSCPxj
Copy link
Collaborator

The linked travis build is passing, but it seems github is not noticing it....

@cdecker cdecker merged commit 4302afd into ElementsProject:master Jun 18, 2020
@darosior darosior deleted the issue-3591 branch July 28, 2020 16:42
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.

Onchain fee roundoff
4 participants