-
Notifications
You must be signed in to change notification settings - Fork 15
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 coinbase transaction amount returned by filtertransactions #971
Conversation
entry.pushKV("abandoned", wtx.isAbandoned()); | ||
} | ||
|
||
// UNIT-E: TODO: get staked transactions |
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.
For the reason why this branch was removed, see #779 (comment).
src/wallet/rpcwalletext.cpp
Outdated
output.pushKV("amount", ValueFromAmount(s.amount)); | ||
outputs.push_back(output); | ||
amount -= s.amount; | ||
if (receivedOutputs.count(s.vout) == 0) { |
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.
I think the implementation of GetAmounts
is wrong as it can put one output both to sent and received lists. But maybe it's needed to show internal transfers.
Although I understand the spirit of the change, I'm not sure about changing the meaning of the |
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.
@cmihai can you verify this change on top of the already running alpha testnet?
Currently, a transaction amount and fee are calculated by the following algorithm: * The 'fee' is the difference between a transaction's sum of outputs and its sum of inputs. * The 'amount' is the difference between the sum of its outputs and the sum of those outputs that are sent to the node itself. This makes sense for a regular transaction, however, for a coinbase Tx, the amount will be zero, because all of its outputs are sent to the node itself. This commit special-cases coinbase Txs and sets the amount to the fee itself, which is what most people expect to see. Signed-off-by: Mihai Ciumeica <mihai@thirdhash.com>
8c46560
to
abbe7e0
Compare
@castarco that's a good idea, but it's not a one-line change since 1) for now, |
Signed-off-by: Mihai Ciumeica <mihai@thirdhash.com>
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 36ea74b
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 36ea74b
This commit cherry-picks the
GetAmount
changes from #811 and fixes thefiltertransactions
RPC output for coinbase transactions, setting the "fee" field to zero and "amount" to the reward received by the miner. Previously, as a side-effect of the algorithm, it was the other way around.See also #779 and #961.