Skip to content
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

[Tests] Add prioritisetransaction RPC test #7063

Closed

Conversation

sdaftuar
Copy link
Member

@sdaftuar sdaftuar commented Nov 19, 2015

@morcos mentioned in #6898 (comment) that there's no rpc test for prioritisetransaction. This adds a simple test to exercise this logic.

In writing this test I discovered something unusual -- if you try to set a fee delta using prioritisetransaction that is negative and greater in magnitude than the transaction's fee, you get incorrect results. The CFeeRate constructor that takes a fee and size doesn't work if you pass in a negative fee. I don't really understand the details of how conversion and type promotion work in this kind of situation, but as far as I can tell, on my machine, I think the compiler promotes the int64_t nFeePaid to a uint64_t (nSize is a size_t):

nSatoshisPerK = nFeePaid*1000/nSize;

Link to the code:

nSatoshisPerK = nFeePaid*1000/nSize;

I think this arithmetic may work on a 32-bit platform, because it looked to me like doing arithmetic with an unsigned int rather than a size_t appeared to work.

Anyway I expect that #6898 will fix prioritisetransaction to work properly with fee deltas that bring the modified fee below 0, so I just avoided that issue for now in this test; I can update it in the future to include that case after #6898.

mempool = self.nodes[0].getrawmempool()
assert(txids[0][0] not in mempool)
assert(txids[0][1] in mempool)
print "Prioritised transaction mined: success"
Copy link
Member

@MarcoFalke MarcoFalke Nov 19, 2015

Choose a reason for hiding this comment

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

Nit: Shouldn't the print appear in front of assert? So when it fails, we know where it did (Not just by line number)?

Copy link
Member Author

@sdaftuar sdaftuar Nov 19, 2015

Choose a reason for hiding this comment

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

Well then it would be printing something incorrect if the assertion failed. I guess I could add a different print statement above the asserts, but not sure it's worth it? If the assertion fails I think the stack trace is informative enough, and I don't think the style here is inconsistent with the rest of the tests.

Copy link
Member

@MarcoFalke MarcoFalke Nov 19, 2015

Choose a reason for hiding this comment

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

Where would you put a comment, when you had put one? Then just replace the # with print and you get in L109:

print "Assert that prioritised transaction was mined"

Copy link
Member

@MarcoFalke MarcoFalke Nov 19, 2015

Choose a reason for hiding this comment

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

rest of the tests

I see no reason to do this. You could as well just remove the print completely.

@sipa
Copy link
Member

sipa commented Nov 28, 2015

utACK

@sdaftuar sdaftuar force-pushed the add-prioritisetransaction-rpctest branch from 4c6b6f3 to 596e839 Compare Nov 30, 2015
@sdaftuar
Copy link
Member Author

sdaftuar commented Nov 30, 2015

Rebased, moved prints

laanwj added a commit that referenced this issue Dec 1, 2015
2b31ab9 Add rpc test for prioritisetransaction (Suhas Daftuar)
6e8b07f Add rounding helper function to util.py (Suhas Daftuar)
@laanwj
Copy link
Member

laanwj commented Dec 1, 2015

Merged via 6abf6eb (needed trivial rebase change in rpc-tests.py)

@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 1, 2015

utACK 6abf6eb

@MarcoFalke
Copy link
Member

MarcoFalke commented Dec 1, 2015

PR can be closed!?

@laanwj laanwj closed this Dec 1, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants