Use 2 hour grace period for key timestamps in importmulti rescans

ryanofsky commented at 11:06 pm on February 14, 2017: member

@gmaxwell pointed out the lack of grace period in #9490#issue-199407998.

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

gmaxwell commented at 0:37 am on February 15, 2017: contributor

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 commented at 6:48 am on February 15, 2017: member

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.

laanwj added the label Wallet on Feb 15, 2017

laanwj added the label RPC/REST/ZMQ on Feb 15, 2017

in src/wallet/rpcdump.cpp: in 34cafb1204 outdated

1071@@ -1072,7 +1072,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1072     }
1073 
1074     if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) {
1075-        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp) : chainActive.Genesis();
1076+        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp - 7200) : chainActive.Genesis();

morcos commented at 1:01 pm on February 15, 2017:

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

ryanofsky commented at 2:41 pm on February 16, 2017:

Added max call.

ryanofsky commented at 2:24 pm on February 15, 2017: member

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 force-pushed on Feb 15, 2017

ryanofsky commented at 10:32 pm on February 15, 2017: member

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 commented at 8:12 am on February 16, 2017: member

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 commented at 12:23 pm on February 16, 2017: contributor

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.

[qa] Simplify import-rescan.py

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.

8be0866883

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

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

Use 2 hour grace period for key timestamps in importmulti rescans

Gregory Maxwell <greg@xiph.org> pointed out the lack of grace period in
https://github.com/bitcoin/bitcoin/pull/9490#issue-199407998.

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

e662af3583

ryanofsky force-pushed on Feb 16, 2017

ryanofsky commented at 3:25 pm on February 16, 2017: member

Added max call and updated RPC help text.

morcos commented at 3:41 pm on February 16, 2017: member

utACK e662af3

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

in src/wallet/rpcdump.cpp: in e662af3583

1072@@ -1072,7 +1073,7 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
1073     }
1074 
1075     if (fRescan && fRunScan && requests.size() && nLowestTimestamp <= chainActive.Tip()->GetBlockTimeMax()) {
1076-        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(nLowestTimestamp) : chainActive.Genesis();
1077+        CBlockIndex* pindex = nLowestTimestamp > minimumTimestamp ? chainActive.FindEarliestAtLeast(std::max<int64_t>(nLowestTimestamp - 7200, 0)) : chainActive.Genesis();

theuni commented at 4:55 pm on February 16, 2017:

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.

ryanofsky commented at 5:57 pm on February 16, 2017:

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.

theuni commented at 6:25 pm on February 16, 2017:

Sounds good.

laanwj added this to the milestone 0.14.0 on Feb 16, 2017

jtimon commented at 9:05 pm on February 16, 2017: contributor

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 commented at 11:52 am on February 17, 2017: member

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.

laanwj merged this on Feb 17, 2017

laanwj closed this on Feb 17, 2017

laanwj referenced this in commit 9828f9a996 on Feb 17, 2017

codablock referenced this in commit c7814b6e97 on Jan 31, 2018

codablock referenced this in commit 995f80db97 on Jan 31, 2018

codablock referenced this in commit 16ed1f92a8 on Jan 31, 2018

codablock referenced this in commit 1ba1256217 on Feb 1, 2018

andvgal referenced this in commit 457a9fc55b on Jan 6, 2019

CryptoCentric referenced this in commit 7f7667547e on Feb 28, 2019

CryptoCentric referenced this in commit 1255bca9a5 on Mar 2, 2019

DrahtBot locked this on Sep 8, 2021

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