Improve ScanForWalletTransactions return value #12275
Conversation
|
I was originally going to follow up on this change by making RescanFromTime rescan any missed blocks if a scan stopped early because of a reorg (8add443), but I don't think this should be needed because notifications from the reorg should scan all the relevant blocks. |
There was a problem hiding this comment.
Concept ACK.
Update comment?:
bitcoin/src/wallet/rpcwallet.cpp
Line 3511 in 2ae7cf8
Edit: have you considered keeping the return value but adding a CWallet::ScanResult* scan_result = nullptr parameter?
|
Thanks for the follow up. |
I don't think it should block 0.16, but technically there is a minor regression in 0.16 which this fixes. Since #11281, if you call importmulti with rescan=True during a re-org and are very unlucky, there is a chance you could see "Rescan failed for key..." errors returned without this fix. The errors are spurious and do not indicate a real problem. It'd also be possible the fix the problem with a one line-fix, just dropping this line: Line 1698 in 2ae7cf8 |
| if (failedBlock) { | ||
| return failedBlock->GetBlockTimeMax() + TIMESTAMP_WINDOW + 1; | ||
| } | ||
| } | ||
| return startTime; | ||
| } | ||
|
|
||
| void UpdateResult(CWallet::ScanResult& result, const CBlockIndex& block, bool failed) |
Where is this extra return information (intended to) be used?
I prefer returning a tuple or structure to that kind of quasi out-args. |
Return more information about scan status from ScanForWalletTransactions and try to describe the return value more clearly. There is a slight change in behavior here where rescans that end early due to a reorg will no longer trigger errors. (I incorrectly suggested that that they should trigger errors in the PR where this behavior was introduced: bitcoin#11281 (comment))
|
In either case, I would bias toward only returning values that are used by the callers and add other fields as needed, in the spirit of YAGNI. |
|
Ok, will close. Regarding YAGNI, I was using the returned status to implement a retry loop in scanfortime which would handle reorgs, but I dropped that commit because I figured incomplete rescans were probably harmless with regular block notifications enabled. |
…ing scan result: success / failure / user_abort bd3b036 Add stop_block out arg to ScanForWalletTransactions (Ben Woosley) 3002d6c Return a status enum from ScanForWalletTransactions (Ben Woosley) bb24d68 Make CWallet::ScanForWalletTransactions args and return value const (Ben Woosley) Pull request description: Return the failed block as an out arg. Fixes #11450. /cc #12275 Tree-SHA512: 6a523e5425ebfe24e664a942ae21c797ccc1281c25b1bf8d02ad95c19dae343fd8051985ef11853474de7628fd6bed5f15190fbc087c3466ce6fdecab37d72a9
…indicating scan result: success / failure / user_abort bd3b036 Add stop_block out arg to ScanForWalletTransactions (Ben Woosley) 3002d6c Return a status enum from ScanForWalletTransactions (Ben Woosley) bb24d68 Make CWallet::ScanForWalletTransactions args and return value const (Ben Woosley) Pull request description: Return the failed block as an out arg. Fixes bitcoin#11450. /cc bitcoin#12275 Tree-SHA512: 6a523e5425ebfe24e664a942ae21c797ccc1281c25b1bf8d02ad95c19dae343fd8051985ef11853474de7628fd6bed5f15190fbc087c3466ce6fdecab37d72a9
Return more information about scan status from ScanForWalletTransactions and
try to describe the return value more clearly.
There is a slight change in behavior here where rescans that end early due to a
reorg will no longer trigger errors. (I incorrectly suggested that that they
should trigger errors in the PR where this behavior was introduced:
#11281 (comment))