Backports 0.15 pr26 #3096

PastaPastaPasta · 2019-09-18T23:56:54Z

this should be the final backporting pr for 0.15, there are likely a couple of 0.16 backports that should be included in dash 0.14.1 for stability / general bug fixing reasons

06bcdb8 Convert named argument from nblocks to conf_target (Alex Morcos) 439c4e8 Improve api to estimatesmartfee (Alex Morcos) Pull request description: Through 0.14 branch, the estimatesmartfee API was tagged "WARNING: This interface is unstable and may disappear or change!" and this warning is removed for 0.15, so any wanted API updates should happen now. The changes here are to make the additional parameter for conservative estimates a more general estimate_mode string , to omit the feerate and include an error string instead of returning -1 on error, and to do better parameter checking initially. ~It is only the last 2 commits, but it's built on bitcoin#10706 and bitcoin#10543~. See bitcoin#10707 (comment) for renaming of nblocks argument to conf_target. Will need to be included before string freeze. PR description edited for clarity Tree-SHA512: 6d8ebee8bb410e2950ffd59663eebfed8d1611d995dc935bb91e430d9da7e2f306796f45631458376027d26341c660f09e825e61748103d2f2736ec6dc3df3ae

1fc8c3d No longer ever reuse keypool indexes (Matt Corallo) Pull request description: This fixes an issue where you could reserve a keypool entry, then top up the keypool, writing out a new key at the given index, then return they key from the pool. This isnt likely to cause issues, but given there is no reason to ever re-use keypool indexes (they're 64 bits...), best to avoid it alltogether. Builds on bitcoin#10235, should probably get a 15 tag. Tree-SHA512: c13a18a90f1076fb74307f2d64e9d80149811524c6bda259698ff2c65adaf8c6c3f2a3a07a5f4bf03251bc942ba8f5fd33a4427aa4256748c40b062991682caf

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

a5ecaf1 Fix misspellings and remove safety verbiage (Steven D. Lander) Pull request description: Standardizing punctuation on CLI output and also including a few fixes for grammer. This PR is for text only changes and includes no code edits. Tree-SHA512: afde551bf1212838822188b6723f2bf1b7222decfa1cd7aa6b04967489108a29f80833af6059252af028c53437755f258275af0614e0d4d0311e09421cd8e131

c0025d0 Fix segfault when shutting down before fully loading (Matt Corallo) 1385697 Order chainstate init more logically. (Matt Corallo) ff3a219 Call RewindBlockIndex even if we're about to run -reindex-chainstate (Matt Corallo) b0f3249 More user-friendly error message if UTXO DB runs ahead of block DB (Matt Corallo) eda888e Fix some LoadChainTip-related init-order bugs. (Matt Corallo) Pull request description: This does a number of things to clean up chainstate init order, fixing some issues as it goes: * Order chainstate init more logically - first all of the blocktree-related loading, then coinsdb, then pcoinsTip/chainActive. Only create objects as needed. * More clearly document exactly what is and isn't called in -reindex and -reindex-chainstate both with comments noting calls as no-ops and by adding if guards. * Move the writing of fTxIndex to LoadBlockIndex - this fixes a bug introduced in d6af06d where InitBlockIndex was writing to fTxIndex which had not yet been checked (because LoadChainTip hadn't yet initialized the chainActive, which would otherwise have resulted in InitBlockIndex being a NOP), allowing you to modify -txindex without reindex, potentially corrupting your chainstate! * Rename InitBlockIndex to LoadGenesisBlock, which is now a more natural name for it. Also check mapBlockIndex instead of chainActive, fixing a bug where we'd write the genesis block out on every start. * Move LoadGenesisBlock further down in init. This is a more logical location for it, as it is after all of the blockindex-related loading and checking, but before any of the UTXO-related loading and checking. * Give LoadChainTip a return value - allowing it to indicate that the UTXO DB ran ahead of the block DB. This just provides a nicer error message instead of the previous mysterious assert(!setBlockIndexCandidates.empty()) error. * Calls ActivateBestChain in case we just loaded the genesis block in LoadChainTip, avoiding relying on the ActivateBestChain in ThreadImport before continuing init process. * Move all of the VerifyDB()-related stuff into a -reindex + -reindex-chainstate if guard. It couldn't do anything useful as chainActive.Tip() would be null at this point anyway. Tree-SHA512: 3c96ee7ed44f4130bee3479a40c5cd99a619fda5e309c26d60b54feab9f6ec60fabab8cf47a049c9cf15e88999b2edb7f16cbe6819e97273560b201a89d90762

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

…conditon" → "condition". 5a6671c Fix typo: "conditon" → "condition" (practicalswift) 35aff43 Remove unused variable int64_t nEnd (practicalswift) Pull request description: * Remove unused variable `int64_t nEnd`. Last use of `nEnd` removed in commit 1fc8c3d. * Fix typo: "conditon" → "condition". Typo introduced in commit 439c4e8. Tree-SHA512: 61624e6f70828c485fe46dbe00df76f1a07b7a5849d41bf7d279323b687420e60e9b85192f611a37211f17f3dea8eb3f6f6dc65d90c92e5516404fd81d37785a

…, should test) Signed-off-by: Pasta <pasta@dashboost.org>

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

PastaPastaPasta · 2019-09-19T02:11:36Z

tests pass locally soo... ¯\(ツ)/¯

src/validation.cpp

src/wallet/wallet.cpp

PastaPastaPasta · 2019-09-19T14:41:58Z

resolved all comments, tests passing locally, can squash some commits/remove the initial & reverts on utACK

codablock · 2019-09-20T09:44:19Z

Didn't do a full review yet, but I noticed that 6478562 is included. I backported the same PR, but based on #3101 to avoid the nasty merge conflicts. @Pasta what do you think about removing this specific backport from this PR and instead use my version (which I'll submit in a few minutes)?

UdjinM6 · 2019-09-20T10:51:13Z

I like the #3101 way too (need to revert both 6478562 and 8a216cc in this case). This should probably help with future backports too.

UdjinM6 · 2019-09-20T13:52:41Z

Pls see UdjinM6@0f96089 (note, this is after reverting 6478562 and 8a216cc, the later has an additional issue as it turned out).

PastaPastaPasta · 2019-09-20T15:29:00Z

dropped those two commits and cherry-picked Udjin's fix

e7539f8 Fix some broken init-time prints/constants (Matt Corallo) 13ab353 Check for empty coinsview instead of just-reset coinsview in init (Matt Corallo) fce3f4f Fix resume-of-reindex-after-restart (Matt Corallo) efac91e Always wait for threadGroup to exit in bitcoind shutdown (Matt Corallo) Pull request description: This is a follow-on to bitcoin#10758 to help move 10758 along. The first fixes a regression in master that was partially fixed in 10758, the second I'm not sure if its a regression or not, but its clearly a bug that should be fixed. Tree-SHA512: aca7b97a97dca66e1a218a33cc6f4aa002292ff1bb0af64e35b81fbaa91b9504f2605375808b43e93a63fc73634ad079b30ef6c9f4ba338d3b5f72d816dfeaff

d34957e [wallet] [tests] Add keypool topup functional test (Jonas Schnelli) 095142d [wallet] keypool mark-used and topup (John Newbery) c25d90f [wallet] Add HasUnusedKeys() helper (John Newbery) f2123e3 [wallet] Cache keyid -> keypool id mappings (John Newbery) 83f1ec3 [wallet] Don't hold cs_LastBlockFile while calling setBestChain (John Newbery) 2376bfc [wallet] [moveonly] Move LoadKeyPool to cpp (Matt Corallo) cab8557 [wallet] [moveonly] Move CAffectedKeysVisitor (Jonas Schnelli) Pull request description: This PR contains the first part of bitcoin#10882 : - if a key from the keypool is used, mark all keys up to that key as used, and then try to top up the keypool - top up the keypool on startup Notably, it does not stop the node or prevent the best block from advancing if the keypool drops below a threshold (which means that transactions may be missed and funds lost if restoring from an old HD wallet backup). Tree-SHA512: ac681fefeaf7ec2aab2fa1da93d12273ea80bd05eb48d7b3b551ea6e5d975dd97ba7de52b7fba52993823280ac4079cc36cf78a27dac708107ebf8fb6326142b

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

… checkpoints 85c82b5 Avoid masking of difficulty adjustment errors by checkpoints (Pieter Wuille) Pull request description: Currently difficulty adjustment violations are not reported for chains that branch off before the last checkpoint. Change this by moving the checkpoint check after the difficulty check. Tree-SHA512: 33666f2c3459151b28c42041a463779e6df18f61d3dd5b1879a0af4e5b199ef74d1e33e06af68bebfdfb211569ad5fb56556bfebe9d63b5688d910ea211b839a

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

@ryanofsky

67ceff4 [wallet] Add logging to MarkReserveKeysAsUsed (John Newbery) 1221f60 [wallet] Remove keypool_topup_cleanups (John Newbery) Pull request description: A couple of minor cleanups suggested by @ryanofsky here: bitcoin#11022 (review) Does not affect functionality. Not required for v0.15. Tree-SHA512: d8d0698fd26ea49a4157e68669d5511095760c3a1ecfa3f917e3f273efbafb55c51a202d677614216eae3f796b6e8d17506b2ec2d4799a94f18981b396e65eec

PastaPastaPasta · 2019-09-20T15:30:34Z

dropped the original + revert commits

UdjinM6

Looks good imo 👍

utACK

codablock

See inline comments

codablock · 2019-09-22T11:10:17Z

src/wallet/wallet.cpp

-                nEnd = std::max(nEnd, *(--setExternalKeyPool.end()) + 1);
-            }
-            // TODO: implement keypools for all accounts?
-            if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(walletdb, 0, fInternal), fInternal))) {


I see you fixed the argument discrepancy in a later fix commit, but you omitted the TODO comment that you also removed by accident. We should either re-add that comment or figure out how to deal with multi-account derivation. My current assumption is that we can actually ignore multi-accounts and always use 0 (and thus change the GenerateNewKey method).

codablock · 2019-09-22T11:19:01Z

src/init.cpp

                pcoinsTip = new CCoinsViewCache(pcoinscatcher);
-                LoadChainTip(chainparams);

                deterministicMNManager->UpgradeDBIfNeeded();


UpgradeDBIfNeeded must happen after LoadChainTip, otherwise it becomes a no-op.

codablock · 2019-09-22T11:42:51Z

src/wallet/wallet.cpp

            }

+            m_pool_key_to_index[pubkey.GetID()] = index;
+


NIT: this empty line was not added in the original PR

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

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

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

codablock

utACK

UdjinM6

re-utACK

sipa and others added 11 commits September 18, 2019 17:30

internal -> fInternal

11e41b9

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

add a zero to the GenerateNewKey call

75e8e0a

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

remove rewinding blocks

714438a

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

change nEnd to index (might break the progress bar on wallet creation…

05ad6d2

…, should test) Signed-off-by: Pasta <pasta@dashboost.org>

remove chainparams from method call (10758)

9f71ab0

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

add gargs

1f06ecd

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

UdjinM6 added this to the 14.1 milestone Sep 19, 2019

UdjinM6 requested changes Sep 19, 2019

View reviewed changes

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

src/validation.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

PastaPastaPasta requested a review from UdjinM6 September 19, 2019 16:31

PastaPastaPasta force-pushed the backports-0.15-pr26 branch from 6946848 to 7ae8cae Compare September 20, 2019 15:28

laanwj and others added 8 commits September 20, 2019 10:30

fix

2d922f5

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

remove LogPrintf("Initializing databases...\n");

3d44562

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

revert unintentional change, 10758

ac07bf6

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

Fix remaining issues

4b579c7

PastaPastaPasta force-pushed the backports-0.15-pr26 branch from 7ae8cae to 4b579c7 Compare September 20, 2019 15:30

UdjinM6 previously approved these changes Sep 20, 2019

View reviewed changes

UdjinM6 requested a review from codablock September 21, 2019 14:57

codablock suggested changes Sep 22, 2019

View reviewed changes

PastaPastaPasta added 3 commits September 22, 2019 09:26

add back the todo

64dde0f

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

move deterministicMNManager->UpgradeDBIfNeeded(); to be after LoadC…

f7a9b0d

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

resolve NIT, remove extra line

a72fc7c

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

PastaPastaPasta dismissed UdjinM6’s stale review via a72fc7c September 22, 2019 14:30

PastaPastaPasta requested review from UdjinM6 and codablock September 22, 2019 14:30

codablock approved these changes Sep 22, 2019

View reviewed changes

UdjinM6 approved these changes Sep 22, 2019

View reviewed changes

UdjinM6 merged commit 78a7ed8 into dashpay:develop Sep 22, 2019

UdjinM6 mentioned this pull request Sep 27, 2019

Backport same PRs as done in bitcoin#11447 #3111

Merged

PastaPastaPasta deleted the backports-0.15-pr26 branch October 1, 2019 16:41

This was referenced Oct 1, 2019

RPC backports dash-docs/dash-docs#161

Merged

RPC - Add combinerawtransaction dash-docs/dash-docs#162

Merged

Backports 0.15 pr26 #3096

Backports 0.15 pr26 #3096

Uh oh!

Conversation

PastaPastaPasta commented Sep 18, 2019

Uh oh!

PastaPastaPasta commented Sep 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PastaPastaPasta commented Sep 19, 2019

Uh oh!

codablock commented Sep 20, 2019

Uh oh!

UdjinM6 commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UdjinM6 commented Sep 20, 2019

Uh oh!

PastaPastaPasta commented Sep 20, 2019

Uh oh!

PastaPastaPasta commented Sep 20, 2019

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

codablock left a comment

Choose a reason for hiding this comment

Uh oh!

codablock Sep 22, 2019

Choose a reason for hiding this comment

Uh oh!

codablock Sep 22, 2019

Choose a reason for hiding this comment

Uh oh!

codablock Sep 22, 2019

Choose a reason for hiding this comment

Uh oh!

codablock left a comment

Choose a reason for hiding this comment

Uh oh!

UdjinM6 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

UdjinM6 commented Sep 20, 2019 •

edited

Loading