Improve prioritisetransaction test coverage #12079

pull promag wants to merge 1 commits into bitcoin:master from promag:2018-01-prioritisetransaction changing 1 files +19 −0

promag commented at 1:50 AM on January 3, 2018: member

No description provided.
fanquake added the label Tests on Jan 3, 2018

MarcoFalke commented at 12:25 PM on January 3, 2018: member

in src/rpc/mining.cpp:252 in 021907bf0e outdated
```
 248 | @@ -249,8 +249,6 @@ UniValue prioritisetransaction(const JSONRPCRequest& request)
 249 |              + HelpExampleRpc("prioritisetransaction", "\"txid\", 0.0, 10000")
 250 |          );
 251 |  
 252 | -    LOCK(cs_main);
```
sdaftuar commented at 7:49 PM on January 4, 2018:

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.

promag commented at 8:09 PM on January 4, 2018:

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.

promag commented at 3:35 PM on January 5, 2018:

Done.

MarcoFalke commented at 4:37 PM on January 5, 2018:

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.
[qa] Improve prioritisetransaction functional test 7f67dd0aa6
promag force-pushed on Jan 5, 2018
sdaftuar commented at 4:04 PM on January 5, 2018: member

utACK
sipa commented at 10:36 AM on January 6, 2018: member

utACK 7f67dd0aa67ec030d5793dcc69594173d5b69fcc
MarcoFalke merged this on Jan 6, 2018
MarcoFalke closed this on Jan 6, 2018
MarcoFalke referenced this in commit 45173fa6fc on Jan 6, 2018
PastaPastaPasta referenced this in commit 648708ab48 on Feb 13, 2020
deadalnix referenced this in commit 925cb24b97 on Feb 15, 2020
PastaPastaPasta referenced this in commit e6c3b69693 on Feb 27, 2020
PastaPastaPasta referenced this in commit 5325484fd3 on Feb 27, 2020
PastaPastaPasta referenced this in commit c02a1239b7 on Feb 27, 2020
ckti referenced this in commit 1701cb4d1d on Mar 28, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels