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.

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

Hold mempool.cs for the duration of ATMP. #12368

Merged
merged 2 commits into from Feb 8, 2018
Merged

Hold mempool.cs for the duration of ATMP. #12368

merged 2 commits into from Feb 8, 2018

Conversation

TheBlueMatt
Copy link
Contributor

This resolves an issue where getrawmempool() can race mempool
notification signals. Intuitively we use mempool.cs as a "read
lock" on the mempool with cs_main being the write lock, so holding
the read lock intermittently while doing write operations is
somewhat strange.

This also avoids the introduction of cs_main in getrawmempool()
which reviewers objected to in the previous fix in #12273

@TheBlueMatt
Hold mempool.cs for the duration of ATMP.
85aa839 
This resolves an issue where getrawmempool() can race mempool
notification signals. Intuitively we use mempool.cs as a "read
lock" on the mempool with cs_main being the write lock, so holding
the read lock intermittently while doing write operations is
somewhat strange.
This also avoids the introduction of cs_main in getrawmempool()
which reviewers objected to in the previous fix in bitcoin#12273
@maflcko maflcko mentioned this pull request Feb 6, 2018
Closed
@maflcko maflcko added this to the 0.16.0 milestone Feb 6, 2018
@maflcko
Copy link
Member

maflcko commented Feb 6, 2018

Thanks, fixes the test failures for me. (At least the wallet and mempool related ones, heh)

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

ACK 85aa8398f5d13c659299b81cdae377462b4f8316
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaefz+AAoJENLqSFDnUoslrz4P/07HOkA1I4XAQTGUcFpWYVgM
LdLJIdSY1HcflR/l9L07oNU7wZgKMGRVYMU+fn6sB1FPmJVJAWURSKBBrq1dr37h
AHIkHG4nZAJuxcU1oi+s0n8/KxPh1mLPi+NsTcUm88c1DcWFQQUripYIiSh0f5tv
uFSdIwVYc7es3BdGoSwHhK4d/+tZIZQ9GCUITYfciqiK0YpHSAZ0RF+xM0nHADdC
XDMr0igQ0DmSGrM4yzdbZUciVcxASC+oeM6i5Jgck9fFNIfaQziNIWOk2PBEsAfg
KBj4mkOPjvqmLyxp8FCWaVKxt2VXE1/bQq0pneGkIyJvEri6I39tU0lG6R1ReBm7
WJtSx+XMvsVQU7A8GvcEQ7oCq0pA63T2+YA9p00TRXBYjireDgmhmzrSXkgmxaD8
bw8WZexF1GfBLkKgR4zgjWkYOKvnRqUOKgPoCfTXkyaSxloeOg3pVF/XDMD0A1Yw
pX0ASzgEapm8epJVojF7tLwEEGvzFfaFR/UOA70MHb7k4DtNCjyhgwrjppK3+X32
3BqIzFPaq4w8s4zqJosVKCxP41fzi8uOLPyZetWd2y/JE5dv4FTGuiE5NWIBzyLh
5fGErIBdYAZdQyLQWvCHdJvBVsvscNo/ZL7mXFl4IbdPmUieEz7OnrU82tbb+xMe
ngOWNw5mjJFBtkyLuIme
=ovgc
-----END PGP SIGNATURE-----

@laanwj laanwj added the Mempool label Feb 6, 2018
@maflcko
Copy link
Member

maflcko commented Feb 6, 2018

re-utACK 02fc886 (second commit obviously not for backport)

@promag
Copy link
Member

promag commented Feb 6, 2018

I don't think this is correct because of lock order. You correctly say

// mempool "read lock" (held through GetMainSignals().TransactionAddedToMempool())

And when it hits CWallet::TransactionAddedToMempool the lock order will be cs_main, mempool.cs, cs_wallet.

But, for instance, listtransactions RPC when eventually reaches WalletTxToJSON, the lock order will be cs_main, cs_wallet, mempool.cs.

Correct me if I'm wrong, but can't this create a deadlock?

@TheBlueMatt
Copy link
Contributor Author

There is no lock order across TransactionAddedToMempool as it is called on a background thread (and can wait as long as it wants).

@promag
Copy link
Member

promag commented Feb 6, 2018

Right, GetMainSignals().TransactionAddedToMempool enqueues the signal!

utACK 02fc886.

@sipa
Copy link
Member

sipa commented Feb 7, 2018

utACK 02fc886. We probably want to rethink how locking of chainstate/mempool works later on (either by using actual rwlocks, or by allowing the mempool to lag behind chain validation).

@devrandom
Copy link

utACK 02fc886

@laanwj laanwj merged commit 02fc886 into bitcoin:master Feb 8, 2018
laanwj added a commit that referenced this pull request Feb 8, 2018
@laanwj
Merge #12368: Hold mempool.cs for the duration of ATMP.
d57d10e 
02fc886 Add braces to meet code style on line-after-the-one-changed. (Matt Corallo)
85aa839 Hold mempool.cs for the duration of ATMP. (Matt Corallo)

Pull request description:

  This resolves an issue where getrawmempool() can race mempool
  notification signals. Intuitively we use mempool.cs as a "read
  lock" on the mempool with cs_main being the write lock, so holding
  the read lock intermittently while doing write operations is
  somewhat strange.

  This also avoids the introduction of cs_main in getrawmempool()
  which reviewers objected to in the previous fix in #12273

Tree-SHA512: 29464b9ca3890010ae13b7dc1c53487cc2bc9c3cf3d32a14cb09c8aa33848f57959d8991ea096beebcfb72f062e4e1962f104aefe4252c7db87633bbfe4ab317
laanwj pushed a commit that referenced this pull request Feb 8, 2018
@TheBlueMatt @laanwj
Hold mempool.cs for the duration of ATMP.
b894755 
This resolves an issue where getrawmempool() can race mempool
notification signals. Intuitively we use mempool.cs as a "read
lock" on the mempool with cs_main being the write lock, so holding
the read lock intermittently while doing write operations is
somewhat strange.
This also avoids the introduction of cs_main in getrawmempool()
which reviewers objected to in the previous fix in #12273

Github-Pull: #12368
Rebased-From: 85aa839
Tree-SHA512: 90a505a96cecc065e8575d816f3bb35040df8672efc315f45eb3f2ea086e8ea6ee2c99eed03d0fe2215c8d3ee947a7b120e3c57a25185d03550c9075573ab032
laanwj pushed a commit that referenced this pull request Feb 8, 2018
@TheBlueMatt @laanwj
Add braces to meet code style on line-after-the-one-changed.
3f5012b 
Github-Pull: #12368
Rebased-From: 02fc886
Tree-SHA512: 8e159541f5270801fd3c70540ad3c55e93f0ba37039e651d21f65ba9b271bbbb2f1389b13a0f40fea337e88bb1711f498bb3ee1230ec2c40b6530846ff241a8b
HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Mar 16, 2018
@TheBlueMatt
Hold mempool.cs for the duration of ATMP.
b0149c5 
This resolves an issue where getrawmempool() can race mempool
notification signals. Intuitively we use mempool.cs as a "read
lock" on the mempool with cs_main being the write lock, so holding
the read lock intermittently while doing write operations is
somewhat strange.
This also avoids the introduction of cs_main in getrawmempool()
which reviewers objected to in the previous fix in bitcoin#12273

Github-Pull: bitcoin#12368
Rebased-From: 85aa839
Tree-SHA512: 90a505a96cecc065e8575d816f3bb35040df8672efc315f45eb3f2ea086e8ea6ee2c99eed03d0fe2215c8d3ee947a7b120e3c57a25185d03550c9075573ab032
HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Mar 16, 2018
@TheBlueMatt
Add braces to meet code style on line-after-the-one-changed.
55b7e0b 
Github-Pull: bitcoin#12368
Rebased-From: 02fc886
Tree-SHA512: 8e159541f5270801fd3c70540ad3c55e93f0ba37039e651d21f65ba9b271bbbb2f1389b13a0f40fea337e88bb1711f498bb3ee1230ec2c40b6530846ff241a8b
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 14, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12368: Hold mempool.cs for the duration of ATMP.
22ee217 
02fc886 Add braces to meet code style on line-after-the-one-changed. (Matt Corallo)
85aa839 Hold mempool.cs for the duration of ATMP. (Matt Corallo)

Pull request description:

  This resolves an issue where getrawmempool() can race mempool
  notification signals. Intuitively we use mempool.cs as a "read
  lock" on the mempool with cs_main being the write lock, so holding
  the read lock intermittently while doing write operations is
  somewhat strange.

  This also avoids the introduction of cs_main in getrawmempool()
  which reviewers objected to in the previous fix in bitcoin#12273

Tree-SHA512: 29464b9ca3890010ae13b7dc1c53487cc2bc9c3cf3d32a14cb09c8aa33848f57959d8991ea096beebcfb72f062e4e1962f104aefe4252c7db87633bbfe4ab317
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 14, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12368: Hold mempool.cs for the duration of ATMP.
9c9a1ab 
02fc886 Add braces to meet code style on line-after-the-one-changed. (Matt Corallo)
85aa839 Hold mempool.cs for the duration of ATMP. (Matt Corallo)

Pull request description:

  This resolves an issue where getrawmempool() can race mempool
  notification signals. Intuitively we use mempool.cs as a "read
  lock" on the mempool with cs_main being the write lock, so holding
  the read lock intermittently while doing write operations is
  somewhat strange.

  This also avoids the introduction of cs_main in getrawmempool()
  which reviewers objected to in the previous fix in bitcoin#12273

Tree-SHA512: 29464b9ca3890010ae13b7dc1c53487cc2bc9c3cf3d32a14cb09c8aa33848f57959d8991ea096beebcfb72f062e4e1962f104aefe4252c7db87633bbfe4ab317
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 15, 2020
@laanwj @PastaPastaPasta
Merge bitcoin#12368: Hold mempool.cs for the duration of ATMP.
78d303c 
02fc886 Add braces to meet code style on line-after-the-one-changed. (Matt Corallo)
85aa839 Hold mempool.cs for the duration of ATMP. (Matt Corallo)

Pull request description:

  This resolves an issue where getrawmempool() can race mempool
  notification signals. Intuitively we use mempool.cs as a "read
  lock" on the mempool with cs_main being the write lock, so holding
  the read lock intermittently while doing write operations is
  somewhat strange.

  This also avoids the introduction of cs_main in getrawmempool()
  which reviewers objected to in the previous fix in bitcoin#12273

Tree-SHA512: 29464b9ca3890010ae13b7dc1c53487cc2bc9c3cf3d32a14cb09c8aa33848f57959d8991ea096beebcfb72f062e4e1962f104aefe4252c7db87633bbfe4ab317
@jnewbery jnewbery mentioned this pull request Jan 5, 2021
Merged
@LarryRuane LarryRuane mentioned this pull request Mar 4, 2021
Merged
LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 13, 2021
@TheBlueMatt @LarryRuane
Hold mempool.cs for the duration of ATMP.
a4730c8 
This resolves an issue where getrawmempool() can race mempool
notification signals. Intuitively we use mempool.cs as a "read
lock" on the mempool with cs_main being the write lock, so holding
the read lock intermittently while doing write operations is
somewhat strange.
This also avoids the introduction of cs_main in getrawmempool()
which reviewers objected to in the previous fix in #12273

zcash: cherry picked from commit 85aa839
zcash: bitcoin/bitcoin#12368
LarryRuane pushed a commit to LarryRuane/zcash that referenced this pull request Apr 13, 2021
@TheBlueMatt @LarryRuane
Add braces to meet code style on line-after-the-one-changed.
829e801 
zcash: cherry picked from commit 02fc886
zcash: bitcoin/bitcoin#12368
zkbot added a commit to zcash/zcash that referenced this pull request Apr 17, 2021
@zkbot
Auto merge of #5017 - LarryRuane:upstream-locking-0.16, r=str4d
0e36226 
Bitcoin 0.16 locking PRs

These are locking changes from upstream (bitcoin core) release 0.16 (Aug 14, 2017, to Feb 16, 2018), oldest to newest (when merged to the master branch).

Each commit also includes a reference both to the PR and the upstream commit.

- bitcoin/bitcoin#11126
  - Excludes changes to wallet tests that we don't have.
- bitcoin/bitcoin#10916
  - first commit only; second commit already merged by d9fcc2b
- bitcoin/bitcoin#11107
  - Only the last commit.
- bitcoin/bitcoin#11593
- bitcoin/bitcoin#11585
- bitcoin/bitcoin#11618
- bitcoin/bitcoin#10286
  - Only the third and last commits.
- bitcoin/bitcoin#11870
- bitcoin/bitcoin#12330
- bitcoin/bitcoin#12366
- bitcoin/bitcoin#12368
- bitcoin/bitcoin#12333
  - Only the first commit.
@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
No reviews
Assignees
No one assigned
Labels
Mempool
Projects
None yet
Milestone
0.16.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@TheBlueMatt @maflcko @promag @sipa @devrandom @laanwj