Significantly reduce GetTransaction cs_main locking (TheBlueMatt) #13903

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:Mf1808-ReadBlockFromDiskCsMain changing 2 files +3 −4

MarcoFalke commented at 3:50 PM on August 7, 2018: member

Reading a block from disk does not require cs_main, so it can be removed or held shorter in functions that read from disk.

This is the first commit stolen from #11913 without rebase or otherwise touching it.
Significantly reduce GetTransaction cs_main locking 613758d50d
MarcoFalke added the label Refactoring on Aug 7, 2018
gmaxwell commented at 6:32 PM on August 7, 2018: contributor

What happens if pruning removes the block while we're reading it?
DrahtBot commented at 10:57 PM on August 9, 2018: member


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #15159 ([RPC] Update getrawtransaction by amitiuttarwar)
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.
promag commented at 12:10 AM on August 14, 2018: member

What happens if pruning removes the block while we're reading it? @gmaxwell wouldn't the removal fail since the file is opened?
luke-jr commented at 8:45 PM on August 27, 2018: member

@promag At least on Linux ext4, opened files can be removed cleanly. It would just no longer be possible to reopen them from filename.
promag commented at 9:47 PM on August 27, 2018: member

So in that case pruning would succeed and in that case LGTM.

If fs::remove throws then the process terminates, otherwise CBlockIndex state turns wrong — which would require restart to fix?

I'll try to reproduce.

in src/rpc/rawtransaction.cpp:182 in 613758d50d

 178 | @@ -179,6 +179,7 @@ UniValue getrawtransaction(const JSONRPCRequest& request)
 179 |      if (!GetTransaction(hash, tx, Params().GetConsensus(), hash_block, true, blockindex)) {
 180 |          std::string errmsg;
 181 |          if (blockindex) {
 182 | +            LOCK(cs_main);

ryanofsky commented at 9:26 PM on September 12, 2018:

Note: it looks like adding this lock here is just a bugfix, and not directly related to the other changes.

ryanofsky commented at 9:37 PM on September 12, 2018: member

It seems there is a race condition if the block is pruned after GetBlockPos is called and before the block file is actually opened:

https://github.com/bitcoin/bitcoin/blob/8f464549c46db2954d7b64d1feb200eb35f2e7e8/src/validation.cpp#L1094-L1099

On unix it should be sufficient to just hold onto cs_main until after the block file is opened but before it is read, but on windows I'm not sure.
DrahtBot commented at 4:49 PM on January 30, 2019: member

Needs rebase
DrahtBot added the label Needs rebase on Jan 30, 2019
MarcoFalke closed this on Jul 11, 2019
MarcoFalke deleted the branch on Jul 11, 2019
laanwj removed the label Needs rebase on Oct 24, 2019
MarcoFalke locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#11913 Avoid cs_main during ReadBlockFromDisk Calls #25232 rpc: Faster getblock API

Significantly reduce GetTransaction cs_main locking (TheBlueMatt) #13903

Conflicts