Skip to content

Improve prioritisetransaction test coverage #12079

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
merged 1 commit into from
Jan 6, 2018
Merged

Improve prioritisetransaction test coverage #12079

merged 1 commit into from
Jan 6, 2018

Conversation

promag
Copy link
Contributor

@promag promag commented Jan 3, 2018

No description provided.

@fanquake fanquake added the Tests label Jan 3, 2018
@maflcko
Copy link
Member

maflcko commented Jan 3, 2018 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK 416826cd1c9c7edfe2eb77b70e0d38313a8a57aa
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaTMuiAAoJENLqSFDnUoslBEUQAINrbUNb/HR/Jif3TuGxhu0h
RKY+p/M3sRPNIHgcPPK7wjLc7wdJ128hcLIAzNca9R76XXtUoFz5i6/kaA/Dno3g
9jRyTsj6Y17FSTDO24gO8xUkkuieX4BvRAmOxGciQY4Yl8Bz8SzjZ+9Tide3d2ky
Yixt7dI3+nniy5rShey33iKdUzmTPO1LPiM0cW2dYIDEtGXGH0e5yUPXT2XhVkHr
+sRD9OU3egQRrquz5lvXEcjZBUZJ0aPxxTddW4netzRfADV3nqxd05/r1lwu3zw/
ianUdl5rqMkWxYq42+yCiw2qNxH+KcbLB71hncJaIt/atryNMTdEjF6qUw3Hnb66
DJHS/SZT/a/CpONM1IcHcabQZJsfe+RsdcDODfTsGYDsdkERlsh1Ri/96Bs0dFOw
WNcCrbLXTwY2Qdp+OG8MkMxhsbt4B70FEplDShxxVjEPeMr8mXIpHhmeSK1ptl08
h2S/c6Y0Mv/YvgqoxSI+D3ZTmdGTwyESqF49o3lExl+fM+QeR5+xLcFuvHqgIYsm
UPflAajI/660MbCN7h8scvez6tOeTcnTPtJZxPv65OlmS3a7gzLiggoJ1Q9n+V6I
B4GmNA0tb94p2NDr4YNm+qVe8PRxy12R58CZwsKb5GmdM3bqbdRZ8Q6UwM3Etmml
yZqdWJa5nI0uqvcL5i7D
=3ZD6
-----END PGP SIGNATURE-----

sdaftuar
sdaftuar reviewed Jan 4, 2018
View reviewed changes
src/rpc/mining.cpp Outdated
@@ -249,8 +249,6 @@ UniValue prioritisetransaction(const JSONRPCRequest& request)
+ HelpExampleRpc("prioritisetransaction", "\"txid\", 0.0, 10000")
);

LOCK(cs_main);
Copy link
Member

Choose a reason for hiding this comment

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

Why is this being removed? It's not obvious to me that this wouldn't result in some kind of race with AcceptToMemoryPool.

Generally I think of cs_main as required to make any changes to the mempool, while mempool.cs is sufficient for read-only access to the mempool... PrioritiseTransaction changes the mempool (re-sorting it), though I guess doesn't add or remove anything -- so it's not clear to me whether this would be problematic or not, with our current code. But it seems safer to me to not remove the lock here, because even if there's no race condition now, it seems like it could result in a future bug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I can remove that commit, it's not entirely related to this PR. I guess it can be revisited later after annotations are in place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer if we document such assumptions in the code. Preferably with a clang static annotation but maybe also with a short description about the locking requirements for that method/class, if needed.

@promag promag force-pushed the 2018-01-prioritisetransaction branch from 416826c to 7f67dd0 Compare January 5, 2018 15:35
@sdaftuar
Copy link
Member

sdaftuar commented Jan 5, 2018

utACK

@sipa
Copy link
Member

sipa commented Jan 6, 2018

utACK 7f67dd0

@maflcko maflcko merged commit 7f67dd0 into bitcoin:master Jan 6, 2018
maflcko pushed a commit that referenced this pull request Jan 6, 2018
Merge #12079: Improve prioritisetransaction test coverage
45173fa 
7f67dd0 [qa] Improve prioritisetransaction functional test (João Barbosa)

Pull request description:

Tree-SHA512: 7a5c446772069cd9ace085ae2635e1f61870c597e2216614628f4b6ebfe209b29f381a182a6f60d09f43f22bb82b59bb573b5441fa8e7b958a5fd0d5aad80d86
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 13, 2020
@PastaPastaPasta
Merge bitcoin#12079: Improve prioritisetransaction test coverage
648708a 
7f67dd0 [qa] Improve prioritisetransaction functional test (João Barbosa)

Pull request description:

Tree-SHA512: 7a5c446772069cd9ace085ae2635e1f61870c597e2216614628f4b6ebfe209b29f381a182a6f60d09f43f22bb82b59bb573b5441fa8e7b958a5fd0d5aad80d86
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Feb 15, 2020
Merge #12079: Improve prioritisetransaction test coverage
925cb24 
Summary:
7f67dd0 [qa] Improve prioritisetransaction functional test (João Barbosa)

Pull request description:

Tree-SHA512: 7a5c446772069cd9ace085ae2635e1f61870c597e2216614628f4b6ebfe209b29f381a182a6f60d09f43f22bb82b59bb573b5441fa8e7b958a5fd0d5aad80d86

Backport of Core [[bitcoin/bitcoin#12079 | PR12079]]
bitcoin/bitcoin#12079

Test Plan:
  test_runner.py mining_prioritisetransaction

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D5292
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@PastaPastaPasta
Merge bitcoin#12079: Improve prioritisetransaction test coverage
e6c3b69 
7f67dd0 [qa] Improve prioritisetransaction functional test (João Barbosa)

Pull request description:

Tree-SHA512: 7a5c446772069cd9ace085ae2635e1f61870c597e2216614628f4b6ebfe209b29f381a182a6f60d09f43f22bb82b59bb573b5441fa8e7b958a5fd0d5aad80d86
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@PastaPastaPasta
Merge bitcoin#12079: Improve prioritisetransaction test coverage
5325484 
7f67dd0 [qa] Improve prioritisetransaction functional test (João Barbosa)

Pull request description:

Tree-SHA512: 7a5c446772069cd9ace085ae2635e1f61870c597e2216614628f4b6ebfe209b29f381a182a6f60d09f43f22bb82b59bb573b5441fa8e7b958a5fd0d5aad80d86
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Feb 27, 2020
@PastaPastaPasta
Merge bitcoin#12079: Improve prioritisetransaction test coverage
c02a123 
7f67dd0 [qa] Improve prioritisetransaction functional test (João Barbosa)

Pull request description:

Tree-SHA512: 7a5c446772069cd9ace085ae2635e1f61870c597e2216614628f4b6ebfe209b29f381a182a6f60d09f43f22bb82b59bb573b5441fa8e7b958a5fd0d5aad80d86
@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.
Reviewers
2 more reviewers

@maflcko maflcko maflcko left review comments

@sdaftuar sdaftuar sdaftuar 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.
5 participants
@promag @maflcko @sdaftuar @sipa @fanquake