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

fix: swap allowAbsurdFees for maxFeeRate for Dash Core v18 #90

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

coolaj86
Copy link

@coolaj86 coolaj86 commented Sep 14, 2022

Feedback wanted: What should the default maxFeeRate be?

Re: dashpay/insight-ui#75

pshenmic
pshenmic previously approved these changes Sep 14, 2022
Copy link
Collaborator

@pshenmic pshenmic left a comment

Choose a reason for hiding this comment

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

LGTM

lib/services/dashd.js Outdated Show resolved Hide resolved
Copy link
Member

@shumkov shumkov left a comment

Choose a reason for hiding this comment

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

👍

@shumkov shumkov merged commit 95e1606 into dashpay:master Sep 19, 2022
@coolaj86
Copy link
Author

coolaj86 commented Sep 21, 2022

This PR is incomplete.

It took a few days for me to set up a full node to test (33gb of blocks and 50gb of indexes).

Notes for follow up:

node_modules/@dashevo/dashd-rpc/lib/index.js:

- sendRawTransaction: 'str bool bool',
+ sendRawTransaction: 'str float bool',

Re: dashpay/dashd-rpc#50

Even after that change I still get:

Error: "absurdly-high-fee, 383 > 373 (code 256). Code:-26"

Can't figure out where it's coming from.

@coolaj86
Copy link
Author

coolaj86 commented Sep 21, 2022

I updated this from feedback on discord that maxFeeRate was a float, but I'm pretty sure it's supposed to be an int.

When I change from 0.00001000 to 1000 that error goes away.

Also need to investigate @dashevo/dashcore-node/lib/services/web.js for socket io stuff...

@shumkov
Copy link
Member

shumkov commented Sep 21, 2022

@coolaj86 so we need one more PR?

@coolaj86
Copy link
Author

All related PRs have been opened.

I've tested locally going over and under the maximum fee.

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