Skip to content

test: Add remaining scenarios of 0 waste, in wallet waste_test #22938

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.

Sign up for GitHub

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

Jump to bottom
Merged
meshcollider merged 1 commit into from
Sep 28, 2021
Merged

test: Add remaining scenarios of 0 waste, in wallet waste_test #22938

meshcollider merged 1 commit into from
Sep 28, 2021

Conversation

@rajarshimaitra
Copy link
Contributor

@rajarshimaitra rajarshimaitra commented Sep 10, 2021 •
edited by meshcollider
Loading

As per the review club discussion on #22009 , it was observed that there were other two fee scenarios in which selection waste could be zero.

These are:

  • (LTF - Fee) == Change Cost
  • (LTF - Fee) == Excess

Even though these are obvious by the definition of waste metric, adding tests for them can be helpful in explaining its behavior
to new readers of the code base, along with pinning the behavior for future.

This PR adds those two cases to waste calculation unit test.

Also let me know if I am missing more scenarios.

@fanquake fanquake added the Tests label Sep 10, 2021
@rajarshimaitra rajarshimaitra mentioned this pull request Sep 10, 2021
Merged
@theStack
Copy link
Contributor

theStack commented Sep 10, 2021

Concept ACK

@murchandamus
Copy link
Contributor

murchandamus commented Sep 10, 2021

Concept ACK

These are:

* (Fee - LTF) == Change Cost

* (Fee - LTF) == Excess

Strictly speaking it should be:

  • (Fee - LTF) + Change Cost = 0
    ⇒ (Fee - LTF) == -1×Change Cost

  • (Fee - LTF) + Excess = 0
    ⇒ (Fee - LTF) == -1×Excess

@achow101
Copy link
Member

achow101 commented Sep 10, 2021

ACK c976ee0a0ca00fe8a7de5c806560da35c2f48729

murchandamus
murchandamus reviewed Sep 10, 2021
View reviewed changes
Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

Left a nit on the comment explaining the test

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Sep 12, 2021

Thanks @xekyo for the review and agreed. It should be reversed.

Updated everywhere..

jonatack
jonatack reviewed Sep 12, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

ACK 0ce33016eb905f5ac3709ed95ae02db0f97885a2

Perhaps update the commit message and pull description with @xekyo's calculations in #22938 (comment). Edit: never mind, it seems fine now IIUC.

@murchandamus
Copy link
Contributor

murchandamus commented Sep 16, 2021

ACK 0ce33016eb905f5ac3709ed95ae02db0f97885a2

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Sep 23, 2021

Thanks @jonatack for the review. Those were good suggestions.

Updated the comment description.
Updated commit message.
Updated with braced initialization.

@murchandamus
Copy link
Contributor

murchandamus commented Sep 23, 2021

Re-ack a2a9c23

jonatack
jonatack reviewed Sep 23, 2021
View reviewed changes
Copy link
Member

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Code review re-ACK a2a9c23e261ad88c0133b407c6e1b4c54f90a860 per git diff 0ce3301 a2a9c23

Sub-atomic consistency nits, but definitely ignore unless you have to rebase or something (and maybe even then).

@rajarshimaitra
test: Add remaining scenarios of 0 waste
efcaefc 
There are two more cases where waste can be 0, when:
 - (Fee - LTF) == -Change Cost
 - (Fee - LTF) == -Excess

Adding these two conditions explicitly in the unit test will help
pin the behavior, also demonstrate waste calculation scenarios to new
readers.
@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Sep 24, 2021 •
edited
Loading

@jonatack Sorry for my silly overlooks.. Updated now only..

@jonatack
Copy link
Member

jonatack commented Sep 24, 2021

Tested re-ACK efcaefc

@achow101
Copy link
Member

achow101 commented Sep 24, 2021

ACK efcaefc

meshcollider
meshcollider reviewed Sep 28, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

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

ACK efcaefc

@meshcollider meshcollider merged commit a8bbd4c into bitcoin:master Sep 28, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Oct 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@jonatack jonatack jonatack left review comments

@meshcollider meshcollider meshcollider left review comments

@murchandamus murchandamus murchandamus left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@rajarshimaitra @theStack @murchandamus @achow101 @jonatack @meshcollider @fanquake