Fix off-by-one error checking coinbase maturity in optional UTxOs #1830
Conversation
efe9b4a to
7d0dd3a
Compare
|
Looks good to me. I would add to Changelog something like this
|
7d0dd3a to
bb18fa7
Compare
…UTxOs The `preselect_utxos` method has an off-by-one error that is making the selection of optional UTxOs too restrictive, by requiring the coinbase outputs to surpass or equal coinbase maturity time at the current height of the selection, and not in the block in which the transaction may be included in the blockchain. The changes in this commit fix it by considering the maturity of the coinbase output at the spending height and not the transaction creation height, this means, a +1 at the considered height at the moment of building the transaction.
bb18fa7 to
03b7eca
Compare
I got confused and added an entry to |
| spendable &= (current_height | ||
| .saturating_sub(anchor.block_id.height)) | ||
| >= COINBASE_MATURITY; | ||
| let spend_height = current_height + 1; |
There was a problem hiding this comment.
Nit: I think a better name would be inclusion_height.
There was a problem hiding this comment.
Won't be needed if #1798 is merged.
Description
As I was developing the changes in #1798 I discover issue #1810. So I introduced the fixes in that PR but later I split them in two to ease the review by suggestion of @oleonardolima .
The
preselect_utxosmethod has an off-by-one error that is making the selection of optional UTxOs too restrictive, byrequiring the coinbase outputs to surpass or equal coinbase maturity time at the current height of the selection, and
not in the block in which the transaction may be included in the blockchain.
The changes in this commit fix it by considering the maturity of the coinbase output at the spending height and not
the transaction creation height, this means, a +1 at the considered height at the moment of building the
transaction.
Fixes #1810.
Notes to the reviewers
Tests for issue #1810 have not been explicitly added, as there already was a
text_spend_coinbasetest which was corrected to ensure coinbase maturation is considered in alignment with the new logic.Changes are not breaking but I'm modifying slightly the documentation for the public method
TxBuilder::current_heightto adjust to the fixed code. Does this merit an entry in the CHANGELOG?Changelog notice
Walletnow considers a utxo originated from a coinbase transaction (coinbase utxo) as available for selection if it will mature in the next block after the height provided to the selection, the current height by default. The previous behavior allowed selecting acoinbase utxoonly when the height at the moment of selection was equal to maturity height or greater.Checklists
cargo fmtandcargo clippybefore committing