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 (review))
Concept ACK.
Update comment?: https://github.com/bitcoin/bitcoin/blob/2ae7cf8ef5be67e085abc1b1dc71bc44865a71b3/src/wallet/rpcwallet.cpp#L3511
Edit: have you considered keeping the return value but adding a CWallet::ScanResult* scan_result = nullptr parameter?
Do we need this for 0.16?
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:
1632 }
1633 }
1634 return startTime;
1635 }
1636
1637+void UpdateResult(CWallet::ScanResult& result, const CBlockIndex& block, bool failed)
Return more information about scan status from ScanForWalletTransactions and try to describe the return value more clearly.
Where is this extra return information (intended to) be used?
Edit: have you considered keeping the return value but adding a CWallet::ScanResult* scan_result = nullptr parameter?
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:
https://github.com/bitcoin/bitcoin/pull/11281#discussion_r163070403)
