Skip to content

Backports 0.16 pr11 #3361

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
UdjinM6 merged 17 commits into from
Mar 19, 2020
Merged

Backports 0.16 pr11 #3361

UdjinM6 merged 17 commits into from
Mar 19, 2020

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Mar 14, 2020

No description provided.

laanwj and others added 5 commits March 14, 2020 00:56
@laanwj @PastaPastaPasta
Merge bitcoin#11710: cli: Reject arguments to -getinfo
0c97cd5 
dcfef27 cli: Reject arguments to -getinfo (Wladimir J. van der Laan)

Pull request description:

  Currently it's possible to accidentally type e.g.

      bitcoin-cli -getinfo getbalance

  and get an answer which can be confusing; the trailing arguments are just ignored.

  To avoid this, throw an error if the user provides arguments to
  `-getinfo`.

Tree-SHA512: 3603e8fa852b884d1dd3b7462db40b092fe8b3390fd4384b4ee330315d797aff711e9f62990012fd4b5a55c8678734ba8497a5488a09ee6b65cf8a99017d6eb4
@laanwj @PastaPastaPasta
Merge bitcoin#10154: init: Remove redundant logging code
26281c0 
faafa80 init: Remove redundant logging code (MarcoFalke)

Tree-SHA512: 5ad0e9aba0e25a36025dd4ee5e5fddd2c0039f95bafd0f33300ea59e2f9bba807da6a1a8b4311d6aad5a360b99163edf4a4f161cb13f0f38580d8d6b504c94ad
@laanwj @PastaPastaPasta
Merge bitcoin#11191: RPC: Improve help text and behavior of RPC-logging.
62987be 
c60c49b Improve help text and behavior of RPC-logging (Akio Nakamura)

Pull request description:

  1. It is allowed `libevent` logging to be updated during runtime,
    but still described that restriction in the help text.
    So we delete these text.
  2. Add a descrption about the evaluation order of `<include>` and
    `<exclude>` to clarify how debug loggig categories to be set.
  3. Add a description about the available logging category `"all"`
    which is not explained.
  4. Add `"optional"` to the help text of `<include>` and `<exclude>`.
  5. Add missing new lines before `"Argument:"`.
  6. `"0"`,`"1"` are allowed in both array of `<include>` and `<exclude>`.
    `"0"` is **ignored** and `"1"` is treated **same as** `"all"`.
    It is confusing, so forbid them.
  7. It always returns all logging categories with status.
    Fix the help text to match this behavior.

Tree-SHA512: c2142da1a9bf714af8ebc38ac0d82394e2073fc0bd56f136372e3db7b2af3b6746f8d6b0241fe66c1698c208c124deb076be83f07dec0d0a180ad150593af415
@PastaPastaPasta
add include
e2c7d1f
@PastaPastaPasta
Merge bitcoin#11714: [tests] Test that mempool rejects coinbase trans…
d8d18d6 
…actions

65e91f5 [tests] Test that mempool rejects coinbase transactions (James O'Beirne)

Pull request description:

  ![selection_063](https://user-images.githubusercontent.com/73197/32978622-b0fa9d70-cbfa-11e7-9a72-1997409e5ba8.png)

  Neither the unit nor functional tests appear to cover rejecting a transaction from acceptance to the mempool on the basis of it being a coinbase. Seems like a decent thing to have a test for.

Tree-SHA512: 53af53c975cad5d7a21c443d71a1c0ced5c70a7799b75bb44d9b7dd6ab2afbcdcaab14571540efeb848f3a1daee5e1dd856530d8f2b50582595219a1c17555ff
@PastaPastaPasta PastaPastaPasta changed the title Backports 0.16 pr11 [WIP] Backports 0.16 pr11 Mar 15, 2020
MarcoFalke and others added 9 commits March 14, 2020 22:01
@PastaPastaPasta
Merge bitcoin#12302: test: Make ua_comment test pass on 0.16.0
1948435 
aac6bce test: Make ua_comment test pass on 0.16.0 (Wladimir J. van der Laan)

Pull request description:

  The specific length of the uacomment is one shorter on `0.16.0` than on `0.15.99` causing the (stupid) test to fail.

  This change makes `assert_start_raises_init_error` optionally take a regexp, so that the error message can be checked without being specific about the reported length.

Tree-SHA512: 1c5e9f1d0b36f004f8113c7e211e8281194ce8057869ac3702172d8b021df3c3aa3b8f79b9434678ed0fb7146f848802410647d434fc884aa200b6a5e26afe8b
@laanwj @PastaPastaPasta
Merge bitcoin#12367: Fix two fast-shutdown bugs
8095699 
dd2de47 Fix fast-shutdown crash if genesis block was not loaded (Matt Corallo)
1c9394a Fix fast-shutdown hang on ThreadImport+GenesisWait (Matt Corallo)

Pull request description:

  The second commit is a much simpler alternative fix for the issue fixed in bitcoin#12349. To test I made ShutdownRequested() always StartShutdown() after a certain number of calls, which turned up one other hang, fixed in the first commit.

Tree-SHA512: 86bde6ac4b8b4e2cb99fff87dafeed02c0d9514acee6d94455637fb2da9ffc274b5ad31b0a6b9f5bd7b700ae35395f28ddb14ffc65ddda3619aa28df28a5607d
@laanwj @PastaPastaPasta
Merge bitcoin#12377: qt: Poll ShutdownTimer after init is done
7e5c5d4 
2222bf0 qt: Poll ShutdownTimer after init is done (MarcoFalke)

Pull request description:

  The shutdown process has started in `requestShutdown`, but initialize will happily continue with `initializeResult` and start threads late in the shutdown progess. Deleting this running thread will crash the application according to the qt docs:
  https://github.com/qt/qtbase/blob/e5033a5c9b769815112e922d0b224af860afd219/src/corelib/thread/qthread.cpp#L412-L415

  Potential fix for bitcoin#12372 (comment)

  This reverts bitcoin#11831 for now and hopefully restores the previous behaviour.

Tree-SHA512: 8e1706afe90ddf2d972aca12c12d4cb2a9a4f38646c59c5466fe5a1a67361896b93c43917d5ac283841ee2bcc62e6bb8dc2bc81dea9129c899b354e9a4ef241b
@laanwj @PastaPastaPasta
Merge bitcoin#12374: qt: Make sure splash screen is freed on AppInitM…
aa34483 
…ain fail

1e5d14b qt: Clarify some comments (Wladimir J. van der Laan)
f5a4c3d qt: Make sure splash screen is freed on AppInitMain fail (Wladimir J. van der Laan)

Pull request description:

  The `splashFinished` event was never sent if AppInitMain fails, causing the splash screen to stick around, causing problems later.

  This bug has existed for a while but is now trigging potential crashed because the splash screen subscribes to wallet events.

  Meant to fix bitcoin#12372.

Tree-SHA512: 192a7e3a528015e771d7860dd95fd7b772292fd8064abf2a3cf3a8ea0d375cd43a6e8ed37ca1a38962fe1410c934599e557adf6a8ef9d87ec7f61b6e5fd8db7e
@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
@laanwj @PastaPastaPasta
Merge bitcoin#12401: Reset pblocktree before deleting LevelDB file
5e19f32 
a8b5d20 Reset pblocktree before deleting LevelDB file (Sjors Provoost)

Pull request description:

  bitcoin#11043 repaced:

  ```
  delete pblocktree;
  pblocktree = new CBlockTreeDB(nBlockTreeDBCache, false, fReset);
  ```

  With:

  ```
  pblocktree.reset(new CBlockTreeDB(nBlockTreeDBCache, false, fReset));
  ```

  This is problematic because `new CBlockTreeDB` tries to delete the existing file, which will fail with `LOCK: already held by process` if it's still open. That's the case for QT.

  When QT finds a problem with the index it will ask the user if they want to reindex. At that point it has already opened `blocks/index`.  It then runs this [while loop](https://github.com/bitcoin/bitcoin/blob/v0.16.0rc3/src/init.cpp#L1415) again with `fReset = 1`, resulting in the above error.

  This change makes that error go away, presumably because `reset()` without an argument closes the file.

Tree-SHA512: fde8b546912f6773ac64da8476673cc270b125aa2d909212391d1a2001b35c8260a8772126b99dfd76b39faaa286feb7c43239185fe584bd4dc2bc04a64044ce
@laanwj @PastaPastaPasta
Merge bitcoin#12415: Interrupt loading thread after shutdown request
99caed2 
2e9406c Interrupt loading thread after shutdown request (João Barbosa)

Pull request description:

  This change (currently) avoids loading the mempool if shutdown is requested.

Tree-SHA512: 3dca3a6ea5b09bd71db0974584d93dfe81819bc0bdbb4d9b6fa0474755306d1403f6c058ecb8211384493a8f7ca3a9134173db744b7344043cfc7d79286c8fd4
@laanwj @PastaPastaPasta
Merge bitcoin#12349: shutdown: fix crash on shutdown with reindex-cha…
b447c5a 
…instate

ceaefdd fix possible shutdown assertion with -reindex-shutdown (Cory Fields)

Pull request description:

  Fixes the assertion error reported here: bitcoin#12349 (comment)

Tree-SHA512: db8e2a275f92a99df7f17852d00eba6df996e412aa3ed3853a9ea0a8cb9800760677532efd52f92abbf2cdcc4210957a87a5f919ac998d46c205365a7a7dffca
@laanwj @PastaPastaPasta
Merge bitcoin#12556: [Trivial] fix version typo in getpeerinfo RPC ca…
ce08cce 
…ll help

d16bfaa fix version typo (Tamas Blummer)

Pull request description:

  RPC getpeeerinfo help output uses non-existent protocol version 7001. Changed to 70001.

Tree-SHA512: aea634d739b51bf26e3f871e8bf2bf812eb62a5d87008ead3ed27ec8bd9b753ad0a958eb21f580e505d7f2b36d60e5d9901dc8e0d25f34b7ef9533fddc898844
@PastaPastaPasta PastaPastaPasta changed the title [WIP] Backports 0.16 pr11 Backports 0.16 pr11 Mar 15, 2020
UdjinM6
UdjinM6 requested changes Mar 16, 2020
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

See inline comments + pls squash fix 11309 into original Merge ... commit

@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Mar 16, 2020

squashed fix 11309 into 11309

UdjinM6
UdjinM6 requested changes Mar 16, 2020
View reviewed changes
src/net_processing.cpp Outdated
Copy link

@UdjinM6 UdjinM6 Mar 16, 2020

Choose a reason for hiding this comment

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

Why? This breaks dev styling guide Align pointers and references to the left i.e. use type& var and not type &var. https://github.com/dashpay/dash/blob/master/doc/developer-notes.md (not to mention that this and changes to whitespaces are unrelated and do not exist in the original PR).

Copy link
Member Author

@PastaPastaPasta PastaPastaPasta Mar 16, 2020

Choose a reason for hiding this comment

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

was this present before the squash? (I didn't actively do this, CLion potentially did it without me realizing)

Copy link
Member Author

@PastaPastaPasta PastaPastaPasta Mar 16, 2020

Choose a reason for hiding this comment

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

applied patch and squashed into 11309

@UdjinM6 UdjinM6 added this to the 16 milestone Mar 16, 2020
@UdjinM6 UdjinM6 added the RPC Some notable changes to RPC params/behaviour/descriptions label Mar 16, 2020
MarcoFalke and others added 3 commits March 16, 2020 16:35
@PastaPastaPasta
Merge bitcoin#11309: Minor cleanups for AcceptToMemoryPool
75f32fb 
bf64c3c Ignore transactions added to mempool during a reorg for fee estimation purposes. (Alex Morcos)
04f78ab Do not reject based on mempool min fee when bypass_limits is set. (Alex Morcos)
fd849e1 Change AcceptToMemoryPool function signature (Alex Morcos)

Pull request description:

  First commit just removes default arguments from `AcceptToMemoryPool` and consolidates two arguments, it does not change behavior.

  Second commit finally fixes the fact that we're not meant to reject based on mempool min fee when adding a transaction from a disconnected block during a reorg as mentioned [here](bitcoin#9602 (comment))

  Third commit makes fee estimation ignore transactions added from a disconnected block during a reorg. I think this was another source of fee estimates returning estimates below 1000 sat/kB as in bitcoin#11303.

Tree-SHA512: 30925ca8b341915bb214f1d2590b36b7931f2e125b7660150e38ae70338f00db5aa7f1608546dddb181446924177eb7cf62ea8bd2583068acc074d6c3f86bc0c

fix 11309

Signed-off-by: Pasta <pasta@dashboost.org>

fix &

Signed-off-by: Pasta <pasta@dashboost.org>
@PastaPastaPasta @UdjinM6
Remove "> 0" from IsArgSet call
9a2fa43 
Co-Authored-By: UdjinM6 <UdjinM6@users.noreply.github.com>
@PastaPastaPasta
Apply the same fix to evoDb and deterministicMNManager as pblocktree
b46693e 
Signed-off-by: Pasta <pasta@dashboost.org>
UdjinM6
UdjinM6 approved these changes Mar 18, 2020
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit 9efa0e3 into dashpay:develop Mar 19, 2020
@PastaPastaPasta PastaPastaPasta deleted the backports-0.16-pr11 branch March 19, 2020 23:39
UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Apr 26, 2020
@UdjinM6
Lock mempool before locking cs_wallet
ee847c7 
This fixes deadlocks introduced in 78d303c/dashpay#3361
UdjinM6 added a commit that referenced this pull request May 2, 2020
@UdjinM6
Fix deadlocks (#3456)
b0963b0 
* Lock mempool before locking cs_wallet

This fixes deadlocks introduced in 78d303c/#3361

* Fix mempool.cs vs cs_inventory potential deadlock

POTENTIAL DEADLOCK DETECTED
Previous lock order was:
 pnode->cs_sendProcessing  net.cpp:2724
 cs_main  net_processing.cpp:3867 (TRY)
 (1) pto->cs_inventory  net_processing.cpp:4106
 (2) cs  txmempool.cpp:1137
Current lock order is:
 cs_main  wallet/rpcwallet.cpp:468
 (2) mempool.cs  wallet/rpcwallet.cpp:468
 pwallet->cs_wallet  wallet/rpcwallet.cpp:469
 cs_main  wallet/wallet.cpp:4045
 (2) mempool.cs  wallet/wallet.cpp:4045
 cs_wallet  wallet/wallet.cpp:4046
 cs_vNodes  net.cpp:3464
 (1) cs_inventory  ./net.h:1056
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Feb 14, 2022
@UdjinM6 @gades
Fix deadlocks (dashpay#3456)
1cfac3e 
* Lock mempool before locking cs_wallet

This fixes deadlocks introduced in 78d303c/dashpay#3361

* Fix mempool.cs vs cs_inventory potential deadlock

POTENTIAL DEADLOCK DETECTED
Previous lock order was:
 pnode->cs_sendProcessing  net.cpp:2724
 cs_main  net_processing.cpp:3867 (TRY)
 (1) pto->cs_inventory  net_processing.cpp:4106
 (2) cs  txmempool.cpp:1137
Current lock order is:
 cs_main  wallet/rpcwallet.cpp:468
 (2) mempool.cs  wallet/rpcwallet.cpp:468
 pwallet->cs_wallet  wallet/rpcwallet.cpp:469
 cs_main  wallet/wallet.cpp:4045
 (2) mempool.cs  wallet/wallet.cpp:4045
 cs_wallet  wallet/wallet.cpp:4046
 cs_vNodes  net.cpp:3464
 (1) cs_inventory  ./net.h:1056
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees

No one assigned

Labels

RPC Some notable changes to RPC params/behaviour/descriptions

Projects

None yet

Milestone

16

Development

Successfully merging this pull request may close these issues.

3 participants

@PastaPastaPasta @UdjinM6 @laanwj