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
test: test RBF rule 3 #25461
test: test RBF rule 3 #25461
Conversation
Concept ACK |
Prior to this change, removing the RBF rule 3 code was not caught by a test failure. Add a testcase to ensure that rule 3 is enforced.
f48305d
to
6de5e66
Compare
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Concept ACK |
Rule 4 implies Rule 3, so isn't this just testing the error string? |
I think we should either remove the "dead" code and fold rule 3 into rule 4 or add this test. Having untested code in RBF policy code does not seem like a good state. |
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 we should either remove the "dead" code and fold rule 3 into rule 4 or add this test.
I think it depends on what we value more:
- To maintain the completeness of the code by explicitly writing conditions for all the rules. Or,
- To keep the code clean, minimal, and free of redundancies.
If there is no predetermined consensus on which option is better, then, in my opinion, I prefer the second option. And so, I favor removing the "dead" code.
I wouldn't call this code "dead" since it is executed, I interpret it as testing debug messages rather than adding coverage of policy code. |
I have never worked on a project before where adding a simple test that provides novel coverage has been conceptually nit-picked to this degree. An "error string" is part of the public interface of this software. We don't know which users are relying on this particular error string. Its continued existence should be verified with a test if practical, which luckily in this case it is. If you want to change the public interface, in principle the right way to do that is with some kind of deprecation schedule. Until then we should not be in a state where we can remove a central chunk of policy code and not have any tests fail. |
No opinion on what to do here. I think anything is fine if reviewers like it. The error string seems to be hit 4 times already in the tests (probably in non-rbf related ones): https://marcofalke.github.io/btc_cov/total.coverage/src/policy/rbf.cpp.gcov.html#169 |
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
I'm in favor of testing what we have, and removing what we don't need. ACK 6de5e66 |
Are you still working on this? Is this still relevant after |
I still think that testing the interface is important. Will rebase if it's not too much trouble otherwise will close. |
Somewhat surprisingly, RBF rule 3 (absolute fees of replacement transaction must meet or exceed original) is untested.
In other words, applying the following patch:
then recompiling and running the full test suite causes no failures.
This is sort of a moot point, because the check following the removed check (rule 4 following rule 3) implicitly requires the absolute fee of the replacement to exceed the original. So effectively, you can't violate rule 3 even if its code is removed.
But still, it seems like we should probably explicitly test for the enforcement of rule 3. This test creates a failure when the above patch is applied.