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

Add option to specify rescan starting timestamp to RPC import calls #6570

Closed
wants to merge 1 commit into from
Closed

Add option to specify rescan starting timestamp to RPC import calls #6570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions src/chain.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,13 @@ const CBlockIndex *CChain::FindFork(const CBlockIndex *pindex) const {
return pindex;
}

CBlockIndex *CChain::FindLatestBefore(int64_t nTime) const {
CBlockIndex *pindex = Tip();
while (pindex && pindex->pprev && pindex->GetBlockTime() > nTime)
Copy link
Contributor

Choose a reason for hiding this comment

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

As already commented, i think we should go down serval blocks (~144) because of possible inaccurate timestamps.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonasschnelli what is the variance on timestamps? Is ~144 enough (or to much)?

Copy link
Contributor

Choose a reason for hiding this comment

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

The cost of rescaning 144 blocks deeper (~1day) is pretty low (+some seconds/minutes). Given that the source of the timestamp is unknown (user could enter it manually because he know when he approx. created the key, or other wallet software did mess around with timezones, etc.) this cost of +144 blocks is probably worth taking.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jonasschnelli However if you know the exact timestamp then that extra time is unnecessary. For a person the offset may be useful, but for a system I think not.

Copy link

Choose a reason for hiding this comment

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

Agreed with @promag, a system can add the safety net of ~144 blocks to the timestamp if necessary and a person likely knows what they are doing if they opt in for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jonasschnelli have to agree with @ruimarinho, this safety net could easily be added by the API user, if desired.
Overall, the only danger for this lies in people using the timestamp aspect of the API instead of a block height.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe just using the median time past is sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using the median time past seems to solve the concern without introducing much extra complexity.

pindex = pindex->pprev;
return pindex;
}

/** Turn the lowest '1' bit in the binary representation of a number into a '0'. */
int static inline InvertLowestOne(int n) { return n & (n - 1); }

Expand Down
3 changes: 3 additions & 0 deletions src/chain.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,9 @@ class CChain {

/** Find the last common block between this chain and a block index entry. */
const CBlockIndex *FindFork(const CBlockIndex *pindex) const;

/** Find the most recent block with timestamp lower than the given. */
CBlockIndex *FindLatestBefore(int64_t nTime) const;
};

#endif // BITCOIN_CHAIN_H
3 changes: 3 additions & 0 deletions src/rpcclient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,12 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "lockunspent", 0 },
{ "lockunspent", 1 },
{ "importprivkey", 2 },
{ "importprivkey", 3 },
{ "importaddress", 2 },
{ "importaddress", 3 },
{ "importaddress", 4 },
{ "importpubkey", 2 },
{ "importpubkey", 3 },
{ "verifychain", 0 },
{ "verifychain", 1 },
{ "keypoolrefill", 0 },
Expand Down
59 changes: 42 additions & 17 deletions src/wallet/rpcdump.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ std::string DecodeDumpString(const std::string &str) {
for (unsigned int pos = 0; pos < str.length(); pos++) {
unsigned char c = str[pos];
if (c == '%' && pos+2 < str.length()) {
c = (((str[pos+1]>>6)*9+((str[pos+1]-'0')&15)) << 4) |
c = (((str[pos+1]>>6)*9+((str[pos+1]-'0')&15)) << 4) |
((str[pos+2]>>6)*9+((str[pos+2]-'0')&15));
pos += 2;
}
Expand All @@ -76,15 +76,16 @@ UniValue importprivkey(const UniValue& params, bool fHelp)
{
if (!EnsureWalletIsAvailable(fHelp))
return NullUniValue;
if (fHelp || params.size() < 1 || params.size() > 3)

if (fHelp || params.size() < 1 || params.size() > 4)
throw runtime_error(
"importprivkey \"bitcoinprivkey\" ( \"label\" rescan )\n"
"importprivkey \"bitcoinprivkey\" ( \"label\" rescan timestamp)\n"
"\nAdds a private key (as returned by dumpprivkey) to your wallet.\n"
"\nArguments:\n"
"1. \"bitcoinprivkey\" (string, required) The private key (see dumpprivkey)\n"
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
"4. timestamp (numeric, optional, default=0) Rescan starts at timestamp\n"
Copy link

Choose a reason for hiding this comment

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

I think the default should be 1231006505 (genesis block), as this is epoch time and therefor not a relative value.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ruimarinho: note that 1231006505 is only the timestamp of the mainnet genesis block, whereby it's 1296688602 for testnet and regtest.

I'm not sure, how the time might be further affected by setmocktime, so I mention it just for completeness.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would leave the default value as 0 meaning Thu, 01 Jan 1970 00:00:00 GMT and mention in the description it is epoch time. I see no point to use the first main block timestamp.

Copy link
Member

Choose a reason for hiding this comment

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

Why not just repurpose the rescan parameter? That is, if it's a boolean, you get the current behaviour, but if it's a Number, you interpret it as a timestamp.

Copy link
Contributor

Choose a reason for hiding this comment

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

@luke-jr that seems less clear and more confusing for no good reason (unless I'm missing the good reason, of course).

Copy link
Member

Choose a reason for hiding this comment

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

@luke-jr So your suggestion is something like "3. rescan (boolean or numeric, optional, default=true) Rescan for wallet transactions starting at the genesis block. A numeric argument is interpreted as the timestamp of the block where to begin to rescan.\n"?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about something like..."3. rescan (numeric, optional, default=1231006505) Rescan for wallet transactions starting at the timestamp provided (main chain genesis block by default). 0 indicates no rescan.\n" ?

Copy link
Member

Choose a reason for hiding this comment

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

Would work for me. And then throw the JSON value is not an integer as expected (code -1) or accept it during a grace period?

Edit: fixed typo per @jtimon

Copy link
Contributor

Choose a reason for hiding this comment

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

you mean "is not numeric as expected"?
I would prefer to do that than to have the argument be "bool or numeric" for some time. If we really want to do the grace period, better create a new one and remove the old one after the grace period.

Copy link
Member

Choose a reason for hiding this comment

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

So your suggestion is something like "3. rescan (boolean or numeric, optional, default=true) Rescan for wallet transactions starting at the genesis block. A numeric argument is interpreted as the timestamp of the block where to begin to rescan.\n"?

@MarcoFalke Exactly. This is clean.

"\nNote: This call can take minutes to complete if rescan is true.\n"
"\nExamples:\n"
"\nDump a private key\n"
Expand Down Expand Up @@ -114,6 +115,14 @@ UniValue importprivkey(const UniValue& params, bool fHelp)
if (params.size() > 2)
fRescan = params[2].get_bool();

CBlockIndex *pindex = chainActive.Genesis();
if (params.size() > 3) {
int nTime = params[3].get_int();
pindex = chainActive.FindLatestBefore(nTime);
if (!pindex)
throw JSONRPCError(RPC_INVALID_PARAMETER, "No block before timestamp");
Copy link
Member

Choose a reason for hiding this comment

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

With the current implementation of FindLatestBefore() this is dead code and can never be thrown?

Copy link
Member

Choose a reason for hiding this comment

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

It's also a useless error - the user doesn't care if there was a block before their timestamp... just rescan the entire chain in that case.

}

CBitcoinSecret vchSecret;
bool fGood = vchSecret.SetString(strSecret);

Expand Down Expand Up @@ -142,7 +151,7 @@ UniValue importprivkey(const UniValue& params, bool fHelp)
pwalletMain->nTimeFirstKey = 1; // 0 would be considered 'no value'

if (fRescan) {
pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true);
pwalletMain->ScanForWalletTransactions(pindex, true);
}
}

Expand Down Expand Up @@ -180,16 +189,17 @@ UniValue importaddress(const UniValue& params, bool fHelp)
{
if (!EnsureWalletIsAvailable(fHelp))
return NullUniValue;
if (fHelp || params.size() < 1 || params.size() > 4)

if (fHelp || params.size() < 1 || params.size() > 5)
throw runtime_error(
"importaddress \"address\" ( \"label\" rescan p2sh )\n"
"importaddress \"address\" ( \"label\" rescan p2sh timestamp)\n"
"\nAdds a script (in hex) or address that can be watched as if it were in your wallet but cannot be used to spend.\n"
"\nArguments:\n"
"1. \"script\" (string, required) The hex-encoded script (or address)\n"
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
"4. p2sh (boolean, optional, default=false) Add the P2SH version of the script as well\n"
"5. timestamp (numeric, optional, default=0) Rescan starts at timestamp\n"
"\nNote: This call can take minutes to complete if rescan is true.\n"
"If you have the full public key, you should call importpublickey instead of this.\n"
"\nExamples:\n"
Expand All @@ -213,6 +223,14 @@ UniValue importaddress(const UniValue& params, bool fHelp)
if (params.size() > 2)
fRescan = params[2].get_bool();

CBlockIndex *pindex = chainActive.Genesis();
if (params.size() > 4) {
int nTime = params[4].get_int();
pindex = chainActive.FindLatestBefore(nTime);
if (!pindex)
throw JSONRPCError(RPC_INVALID_PARAMETER, "No block before timestamp");
}

// Whether to import a p2sh version, too
bool fP2SH = false;
if (params.size() > 3)
Expand All @@ -234,7 +252,7 @@ UniValue importaddress(const UniValue& params, bool fHelp)

if (fRescan)
{
pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true);
pwalletMain->ScanForWalletTransactions(pindex, true);
pwalletMain->ReacceptWalletTransactions();
}

Expand All @@ -248,12 +266,13 @@ UniValue importpubkey(const UniValue& params, bool fHelp)

if (fHelp || params.size() < 1 || params.size() > 4)
throw runtime_error(
"importpubkey \"pubkey\" ( \"label\" rescan )\n"
"importpubkey \"pubkey\" ( \"label\" rescan timestamp)\n"
"\nAdds a public key (in hex) that can be watched as if it were in your wallet but cannot be used to spend.\n"
"\nArguments:\n"
"1. \"pubkey\" (string, required) The hex-encoded public key\n"
"2. \"label\" (string, optional, default=\"\") An optional label\n"
"3. rescan (boolean, optional, default=true) Rescan the wallet for transactions\n"
"4. timestamp (numeric, optional, default=0) Rescan starts at timestamp\n"
"\nNote: This call can take minutes to complete if rescan is true.\n"
"\nExamples:\n"
"\nImport a public key with rescan\n"
Expand All @@ -276,6 +295,14 @@ UniValue importpubkey(const UniValue& params, bool fHelp)
if (params.size() > 2)
fRescan = params[2].get_bool();

CBlockIndex *pindex = chainActive.Genesis();
if (params.size() > 3) {
int nTime = params[3].get_int();
pindex = chainActive.FindLatestBefore(nTime);
if (!pindex)
throw JSONRPCError(RPC_INVALID_PARAMETER, "No block before timestamp");
}

if (!IsHex(params[0].get_str()))
throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey must be a hex string");
std::vector<unsigned char> data(ParseHex(params[0].get_str()));
Expand All @@ -290,7 +317,7 @@ UniValue importpubkey(const UniValue& params, bool fHelp)

if (fRescan)
{
pwalletMain->ScanForWalletTransactions(chainActive.Genesis(), true);
pwalletMain->ScanForWalletTransactions(pindex, true);
pwalletMain->ReacceptWalletTransactions();
}

Expand All @@ -302,7 +329,7 @@ UniValue importwallet(const UniValue& params, bool fHelp)
{
if (!EnsureWalletIsAvailable(fHelp))
return NullUniValue;

if (fHelp || params.size() != 1)
throw runtime_error(
"importwallet \"filename\"\n"
Expand Down Expand Up @@ -388,9 +415,7 @@ UniValue importwallet(const UniValue& params, bool fHelp)
file.close();
pwalletMain->ShowProgress("", 100); // hide progress dialog in GUI

CBlockIndex *pindex = chainActive.Tip();
while (pindex && pindex->pprev && pindex->GetBlockTime() > nTimeBegin - 7200)
pindex = pindex->pprev;
CBlockIndex *pindex = chainActive.FindLatestBefore(nTimeBegin - 7200);

if (!pwalletMain->nTimeFirstKey || nTimeBegin < pwalletMain->nTimeFirstKey)
pwalletMain->nTimeFirstKey = nTimeBegin;
Expand All @@ -409,7 +434,7 @@ UniValue dumpprivkey(const UniValue& params, bool fHelp)
{
if (!EnsureWalletIsAvailable(fHelp))
return NullUniValue;

if (fHelp || params.size() != 1)
throw runtime_error(
"dumpprivkey \"bitcoinaddress\"\n"
Expand Down Expand Up @@ -447,7 +472,7 @@ UniValue dumpwallet(const UniValue& params, bool fHelp)
{
if (!EnsureWalletIsAvailable(fHelp))
return NullUniValue;

if (fHelp || params.size() != 1)
throw runtime_error(
"dumpwallet \"filename\"\n"
Expand Down