Significantly reduce GetTransaction cs_main locking (TheBlueMatt) #13903

1. 
   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.

2. Significantly reduce GetTransaction cs_main locking 613758d50d
3. MarcoFalke added the label Refactoring on Aug 7, 2018
4. 
   gmaxwell commented at 6:32 pm on August 7, 2018: contributor

   What happens if pruning removes the block while we’re reading it?
5. 
   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.

6. 
   promag commented at 0: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?

7. 
   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.
8. 
   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.

9. 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.
10. 
    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.

11. 
    DrahtBot commented at 4:49 pm on January 30, 2019: member
12. DrahtBot added the label Needs rebase on Jan 30, 2019
13. MarcoFalke closed this on Jul 11, 2019

    ---

14. MarcoFalke deleted the branch on Jul 11, 2019
15. laanwj removed the label Needs rebase on Oct 24, 2019
16. MarcoFalke locked this on Dec 16, 2021

Contributors
MarcoFalke gmaxwell DrahtBot promag luke-jr ryanofsky

Labels
Refactoring