Use 2 hour grace period for key timestamps in importmulti rescans #9761

ryanofsky · 2017-02-14T23:06:11Z

@gmaxwell pointed out the lack of grace period in #9490 (comment).

The importwallet RPC which uses key timestamps in a similar way already has a 2 hour grace period.

gmaxwell · 2017-02-15T00:37:17Z

So one consequence of this is that if you use the manual pruning you will potentially prune past the time your imports will attempt to search, and then fail.

laanwj · 2017-02-15T06:48:33Z

Concept ACK.

So one consequence of this is that if you use the manual pruning you will potentially prune past the time your imports will attempt to search, and then fail.

Yes. Though I'm not sure that warrants any changes. After all it's the same for importwallet already.

At the least this grace period needs to be clearly documented in the RPC help so that it can be taken into account.

morcos · 2017-02-15T13:01:55Z

src/wallet/rpcdump.cpp

@@ -1072,7 +1072,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
    }

    if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) {
-        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp) : chainActive.Genesis();
+        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp - 7200) : chainActive.Genesis();


I think this works fine, but does it seem slightly safer to not allow it to go negative?

Added max call.

ryanofsky · 2017-02-15T14:24:44Z

So one consequence of this is that if you use the manual pruning you will potentially prune past the time your imports will attempt to search, and then fail.

Yes. Though I'm not sure that warrants any changes. After all it's the same for importwallet already.

I would want to address this by only adding the grace period when fPruneMode is false. The way I see it, if you're using the rescan feature on a pruned node, you already have to be pretty careful, and while there are real improvements we can make for pruned nodes (like marking keys that could have incomplete transaction lists, or looking for spendable outputs in the utxo set in #9137), adding a grace period for importmulti rescans on pruned nodes is only a slightly useful thing to begin with, so it would be simplest for the code and for users to just keep the current behavior in this case.

At the least this grace period needs to be clearly documented in the RPC help so that it can be taken into account.

Will add a short note.

This change was discussed some on IRC:
https://botbot.me/freenode/bitcoin-core-dev/msg/80989053/
https://botbot.me/freenode/bitcoin-core-dev/msg/81013278/

ryanofsky · 2017-02-15T22:32:35Z

Created #9773 to deal with the issue of pruned nodes. morcos argued against removing the grace period when fPruneMode was false because not having a grace period could potentially lead to missing transaction data, and gmaxwell argued against disabling importmulti rescans on pruned nodes because partial rescanning is an important feature for pruned nodes, so #9773 deals with the issue by returning errors from importmulti when rescans fail because blocks are missing or corrupt.

Still need to add some documentation about the grace period, and add a min call to avoid passing negative values to FindEarliestAtLeast, but the bulk of the code in this PR (i.e. import-rescan.py) is ready for review.

laanwj · 2017-02-16T08:12:19Z

Making the grace period depend on whether the node is pruning or not sounds like a terrible idea to me. It ties together two different options in a completely unexpected way, that's just asking for people to make mistakes.

gmaxwell · 2017-02-16T12:23:31Z

The whole reason manual pruning takes a timestamp is so harmonize it with this, so you can not prune what you need to import. I think this could be addressed by also adding an identical grace to the manual pruning time.

Get rid of partial functions so the test can be more easily extended to add more variants of imports with options that affect rescanning (e.g. different key timestamps). Also change the second half of the test to send /to/ the imported addresses, instead of /from/ the imported addresses. The goal of this part of the test was to confirm that the wallet would pick up new transactions after an import regardless of whether or not a rescan happened during the import. But because the wallet can only do this reliably for incoming transactions and not outgoing transactions (which require the wallet to look up transaction inputs) the test previously was less meaningful than it should have been.

Gregory Maxwell <greg@xiph.org> pointed out the lack of grace period in bitcoin#9490 (comment). The importwallet RPC which uses key timestamps in a similar way already has a 2 hour grace period.

ryanofsky

Added max call and updated RPC help text.

ryanofsky · 2017-02-16T14:41:50Z

src/wallet/rpcdump.cpp

@@ -1072,7 +1072,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
    }

    if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) {
-        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp) : chainActive.Genesis();
+        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp - 7200) : chainActive.Genesis();


Added max call.

morcos · 2017-02-16T15:41:39Z

utACK e662af3

Please tag 0.14
only reviewed code changes did not review test changes

theuni · 2017-02-16T16:55:01Z

src/wallet/rpcdump.cpp

@@ -1072,7 +1073,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
    }

    if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) {
-        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp) : chainActive.Genesis();
+        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(std::max<int64_t>(nLowestTimestamp - 7200, 0)) : chainActive.Genesis();


nit: I think this should be std::max<int64_t>(nLowestTimestamp - 7200, minimumTimestamp)

I only mention because I'm unsure if 0 has a special meaning somewhere.

0 has special meaning for the CWallet::nTimeFirstKey variable, but I recently removed all outside references to that variable and made it private so that no other code that wasn't dealing with it directly would have to worry about special meanings of 0.

minimumTimestamp would work fine if it were set to 0 instead of 1, and I have some local changes to remove this variable, so I didn't want to add another use here.

Sounds good.

jtimon · 2017-02-16T21:05:54Z

Concept ACK, although as commented in #9778, if these grace periods need to be consistent, I think we should have a constant (also used for normal rescan?).

laanwj · 2017-02-17T11:52:44Z

Concept ACK, although as commented in #9778, if these grace periods need to be consistent, I think we should have a constant (also used for normal rescan?).

Yes, would make sense to define a constant. No, that does not need to hold up merging this.

…i rescans e662af3 Use 2 hour grace period for key timestamps in importmulti rescans (Russell Yanofsky) 38d3e9e [qa] Extend import-rescan.py to test imports on pruned nodes. (Russell Yanofsky) c28583d [qa] Extend import-rescan.py to test specific key timestamps (Russell Yanofsky) 8be0866 [qa] Simplify import-rescan.py (Russell Yanofsky)

…ortmulti rescans e662af3 Use 2 hour grace period for key timestamps in importmulti rescans (Russell Yanofsky) 38d3e9e [qa] Extend import-rescan.py to test imports on pruned nodes. (Russell Yanofsky) c28583d [qa] Extend import-rescan.py to test specific key timestamps (Russell Yanofsky) 8be0866 [qa] Simplify import-rescan.py (Russell Yanofsky)

ryanofsky mentioned this pull request Feb 14, 2017

Replace FindLatestBefore used by importmuti with FindEarliestAtLeast. #9490

Merged

laanwj added Wallet RPC/REST/ZMQ labels Feb 15, 2017

morcos reviewed Feb 15, 2017

View reviewed changes

ryanofsky force-pushed the pr/multigrace branch 2 times, most recently from a4ca0ed to f50f403 Compare February 15, 2017 20:19

ryanofsky mentioned this pull request Feb 15, 2017

Return errors from importmulti if complete rescans are not successful #9773

Merged

ryanofsky added 4 commits February 16, 2017 09:05

[qa] Extend import-rescan.py to test specific key timestamps

c28583d

[qa] Extend import-rescan.py to test imports on pruned nodes.

38d3e9e

Use 2 hour grace period for key timestamps in importmulti rescans

e662af3

Gregory Maxwell <greg@xiph.org> pointed out the lack of grace period in bitcoin#9490 (comment). The importwallet RPC which uses key timestamps in a similar way already has a 2 hour grace period.

ryanofsky force-pushed the pr/multigrace branch from f50f403 to e662af3 Compare February 16, 2017 15:21

ryanofsky commented Feb 16, 2017

View reviewed changes

morcos mentioned this pull request Feb 16, 2017

Add two hour buffer to manual pruning #9778

Merged

theuni reviewed Feb 16, 2017

View reviewed changes

laanwj added this to the 0.14.0 milestone Feb 16, 2017

laanwj merged commit e662af3 into bitcoin:master Feb 17, 2017

laanwj mentioned this pull request Feb 17, 2017

[wallet] Remove importmulti always-true check #9760

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use 2 hour grace period for key timestamps in importmulti rescans #9761

Use 2 hour grace period for key timestamps in importmulti rescans #9761

ryanofsky commented Feb 14, 2017

gmaxwell commented Feb 15, 2017

laanwj commented Feb 15, 2017

morcos Feb 15, 2017

ryanofsky Feb 16, 2017

ryanofsky commented Feb 15, 2017 •

edited

Loading

ryanofsky commented Feb 15, 2017 •

edited

Loading

laanwj commented Feb 16, 2017

gmaxwell commented Feb 16, 2017

ryanofsky left a comment

ryanofsky Feb 16, 2017

morcos commented Feb 16, 2017

theuni Feb 16, 2017

ryanofsky Feb 16, 2017

theuni Feb 16, 2017

jtimon commented Feb 16, 2017

laanwj commented Feb 17, 2017

Use 2 hour grace period for key timestamps in importmulti rescans #9761

Use 2 hour grace period for key timestamps in importmulti rescans #9761

Conversation

ryanofsky commented Feb 14, 2017

gmaxwell commented Feb 15, 2017

laanwj commented Feb 15, 2017

morcos Feb 15, 2017

Choose a reason for hiding this comment

ryanofsky Feb 16, 2017

Choose a reason for hiding this comment

ryanofsky commented Feb 15, 2017 • edited Loading

ryanofsky commented Feb 15, 2017 • edited Loading

laanwj commented Feb 16, 2017

gmaxwell commented Feb 16, 2017

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Feb 16, 2017

Choose a reason for hiding this comment

morcos commented Feb 16, 2017

theuni Feb 16, 2017

Choose a reason for hiding this comment

ryanofsky Feb 16, 2017

Choose a reason for hiding this comment

theuni Feb 16, 2017

Choose a reason for hiding this comment

jtimon commented Feb 16, 2017

laanwj commented Feb 17, 2017

ryanofsky commented Feb 15, 2017 •

edited

Loading

ryanofsky commented Feb 15, 2017 •

edited

Loading