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
[trivial] Fix error messages in CFeeBumper #11597
Conversation
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.
Concept ACK.
Nit, reword PR and commit to Fix error messages in CFeeBumper
.
src/wallet/feebumper.cpp
Outdated
vErrors.push_back(strprintf("New fee rate (%s) is less than the minimum fee rate (%s) to get into the mempool. totalFee value should to be at least %s or settxfee value should be at least %s to add transaction.", FormatMoney(nNewFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFeePerK()), FormatMoney(minMempoolFeeRate.GetFee(maxNewTxSize)), FormatMoney(minMempoolFeeRate.GetFeePerK()))); | ||
vErrors.push_back(strprintf( | ||
"New fee rate (%s) is lower than the minimum fee rate (%s) to get into the mempool. " | ||
"The totalFee value should be at least %s or the settxfee value should be at least %s to add transaction.", |
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.
Nit, there are only 2 error messages with periods, this one and:
https://github.com/bitcoin/bitcoin/blob/2c83c511dcda98550b660f6b62da7fab8548ca5c/src/wallet/feebumper.cpp#L293
How about removing the period in the last one and here: "New fee rate ... - the totalFee value should ... to add transaction"
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.
Sounds good -- done.
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.
My suggestion included replacing the inner period with -
2c83c51
to
67a6298
Compare
67a6298
to
a163407
Compare
utACK a16340758e3aa2a73a8b594eeb5fae7f81f9eea1 |
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.
utACK a163407.
src/wallet/feebumper.cpp
Outdated
@@ -284,7 +290,7 @@ bool CFeeBumper::commit(CWallet *pWallet) | |||
// along with an exception. It would be good to return information about | |||
// wtxBumped to the caller even if marking the original transaction | |||
// replaced does not succeed for some reason. | |||
vErrors.push_back("Error: Created new bumpfee transaction but could not mark the original transaction as replaced."); | |||
vErrors.push_back("Error: Created new bumpfee transaction but could not mark the original transaction as replaced"); |
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.
BTW, is there a reason to have these Error:
prefixes?
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.
Good point. Seems to be used only in a few places. I am going to remove the Error:
prefix in the 3 locations it's used.
a163407
to
28eabb0
Compare
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.
utACK 28eabb0.
src/wallet/feebumper.cpp
Outdated
@@ -267,15 +273,15 @@ bool CFeeBumper::commit(CWallet *pWallet) | |||
CValidationState state; | |||
if (!pWallet->CommitTransaction(wtxBumped, reservekey, g_connman.get(), state)) { | |||
// NOTE: CommitTransaction never returns false, so this should never happen. | |||
vErrors.push_back(strprintf("Error: The transaction was rejected! Reason given: %s", state.GetRejectReason())); | |||
vErrors.push_back(strprintf("The transaction was rejected! Reason given: %s", state.GetRejectReason())); |
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.
nit, follow format below (replace !
by :
), check others.
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.
lol.. fixed, thanks.
28eabb0
to
a02c5e4
Compare
utACK a02c5e4 |
utACK a02c5e4 |
a02c5e4 [trivial] Fix error messages in CFeeBumper (Karl-Johan Alm) Pull request description: …pool fee rate. Tree-SHA512: c179853b2a19fdb767e46b29068f3e1ce6db75fda4356746472c93c5b51f0aa495a988c4da1e14762993d57229e525594a2e9d0e089f931c1c67fec7807bda54
…pool fee rate.