The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #21206 (refactor: Make CWalletTx sync state type-safe by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

If this PR is intended to address issue #20181, can you add "Closes #20181" or "Fixes #20181" to the PR description instead of the PR title?

Is it right this way ?

Yes, thank you.

Concept ACK.

Thanks for all the feedback. I'll correct everything and ask question if needed.

Concept ACK. Thanks!

Concept ACK - code looks good (haven't looked at the tests). Code is relatively simple, basically just an if to bypass the smart-time calculation.

I've push a new version with the rework marked as resolved done.
What is not done yet:

• remove hardcoded pubkey in the functionnal test : wait for feedback on the way to do it to limit useless code.
• update ComputeTimeSmart doxygen documentation : wait for feedback on my proposal in comment.
• update AddToWalletIfInvolvingMe doxygen documentation : wait for feedback on my proposal in comment.

I've push the full rework and add documentation update proposal in a separated commit.

Weak NACK. Seems like a hacky "fix" for what is pretty much a user error...?

Thanks for your feedback.

I'll try to give a broader view of the problem and of my fix, from my perspective.

First of all, the current behaviour seems to be present for years, and no recent modification has changed this.
It's rare that users encounter this one and there's clearly no emergency here.

The issue that triggered my investigation is this one : #20181
But by looking at open issue, I've now found two more open issue on Github that should be related to the same behaviour :
#11703
#6662
And one pull request abandoned due to the lack of activity :
#12024
This pull request proposed in 2018 by @achow101 is more radical than the way I fix it, because it remove the SmartTime computation, but this raises an interesting point for me on which I would like to come back later.

The thing is that when someone encounters this behavior from Bitcoin Core wallet, it clearly has no clue on what's going on, because it's hard to reproduce it and the date seems so random and nonsense, that there's no easy way to request for similar problems on search engines. And when you try to investigate it, nothing points you in the right direction.

If it's considered as an error from the RPC API user to proceed first with a recent scan, and after with an older one, it must be documented in the RPC API Reference.
And maybe it's the right solution for this problem : simply inform that rescanblockchain function should be manipulated with some rules in mind.
In that case, the documentation should precise that a miss handling of this method is fatal. Because if the API user proceeds to an improper rescan, there's no way to correct transaction datation later : the only way is to destroy the wallet.

Here why I think it might be a good idea to fix it : the API RPC users are mainly teams working around Bitcoin Core to enrich the ecosystem with more tools.
This part of the ecosystem is far more experimental (and less strict) than Bitcoin Core itself, and that's a good thing from my point of the view.
The specter desktop team has exploited the RPC rescanblockchain function to enable faster wallet import :

• If it's a segwit wallet, the rescanning process starts automatically on the first segwit enable block.
• During the backup process, the first transaction blockheight is saved, to enable auto optimized reimport of this wallet later.
• If the user is a power user and knows the height of his first transaction, it could restart manually from here to restore faster his wallet.

I found all these ideas interesting for a faster and better user experience and guess what, they had a bug that broke my wallet, and pushed me to look at the problem.
This type of experimentation has not his place in the Bitcoin Core wallet, but is, for me, a net positive for the ecosystem.
We could imagine others optimisation for an external wallet project to improve the user experience with this RPC rescanblockchain function, for example : start with a fast utxo rescanning process with UI blocked, then unblock the interface and rescan only the current year, then the previous one, then the previous one,...
From an user perspective, I think that the oldest transactions are way less important than the more recents one.
It's just some random thought, but my core point is that RPC API functions are not going to be used the way the internal wallet uses them, and it's a good thing because it enables leveraged creativity outside the Bitcoin Core project and high quality and reference code for the Bitcoin Core wallet.
That's why I think it might be good to enable a non restricted usage of RPC rescanblockchain function between the two points (start_height and stop_height), even if it's not used by the wallet internally.

Now let's go back to the fix itself.
I've tried to limit the impact on my fix on the existing code, but I'll try to give a broader view of the way I've thought about it to feed the conversation.
As I've said in the description of my PR : In the context of rescanning old blocks, the only time value that has a meaning is the blocktime.
That's why my first reaction was to look for a way not to call the ComputeTimeSmart function during a rescanning of old blocks, because you don't want to launch a complex time evaluation function in the first place when you already know that the blocktime is the only time that means something.
I then discovered that the ComputeTimeSmart call was a very deep call in the hierarchy of functions and that a lot of wallet logic was shared in the middle part whatever the context.
The separation of the code between block rescan processing and realtime processing would have been an incorrect approach, due to this middleware logic.
The main wrong thing here, to me, is to compute the time of a specific context (realtime processing) in a shared function called deep in the hierarchy.
A good way to fix it, is to push this responsibility outside the hierarchy (at least near the surface of the hierarchy).
But this way will require a bigger refactoring for the wallet.cpp file. If someone finds this interesting, I could try to write a proof of concept in a branch outside a pull request. But this patch would have a far bigger impact on the code.

Finally this PR approach was a way of keeping the patch simple while preserving functionality.
But If this seems too hacky, there's 3 other ways :

• Keep it as it's today, don't touch the wallet code, but update the RPC API Reference to explain restrictions on rescanblockchain function.
• Make a bigger refactoring to push ComputeTimeSmart responsibility in the right place in the functions hierarchy.
• Fix it like achow101 in his previous PR, by removing the part of ComputeTimeSmart that is only valid in a realtime context. This way, ComputeTimeSmart has not to be moved because it's not context dependent, but you've restricted a part of the current functionality. It's clearly an interesting approach too, from my point of view because it removes the root cause of the misplacement of the function in the hierarchy call.

Happy to pursue the discussion here.

Seems like a hacky "fix" for what is pretty much a user error...?

How is rescanning blockchain a user error?

Manually scanning out of order, to be specific, is a user error because the wallet is assumed to always be scanned up to its sync-point.

@luke-jr I'd agree the wallet is written with that assumption, but I also don't see a downside to changing the behavior when the wallet can know it's dealing with an unusual situation. I haven't dug into the details, but I suspect this change makes things match expectations in strictly more situations.

Concept ACK

Another thing I think could be done to improve this is to avoid adding these transactions to wtxOrdered when rescanning_old_block is true:

If we want to exclude these transactions from smart time calculation, better to fully exclude instead of half-excluding them. Maybe there is a way to adjust the smart time calculations to include them, but that would require more thought and might not be worth the effort.

Another code cleanup that would be nice to see is moving nTimeReceived, nOrderPos, and wtxOrdered updates inside the ComputeTimeSmart function instead of doing them before. ComputeTimeSmart is only called one place and the current way time calculation is split up outside and inside this function is probably unnecessarily confusing.

IMO, previous ACK is still ok and switching from block time to max block time, excluding these transactions from wtxOrdered, and making ComputeTimeSmart more self-contained are all optional improvements that could be followed up later.

I tried this pull request today, and I still have the same problem. It's pulling in the wrong date after a rescan. I am now attempting to reload the entire blockchain. I need this fixed for my IRS audit.

You must encounter a similar problem as the one that make me code this PR. You can't recovered a wallet with incorrect datation. It's not possible yet, even with this PR : the only way is to destroy and recreate it. This PR only prevent the datation to be corrupted in the first place.

How about nTimeSmart = min(nTimeSmart, block_max_time) and avoid rescanning_old_block argument?

Could you elaborate where do you imagine using it ?

Because min function between nTimeSmart and blocktime is already present in the current Bitcoin Core code and doesn't prevent the original problem.

I'm refering to this line :
nTimeSmart = std::max(latestEntry, std::min(blocktime, latestNow));

If you imagine introduce another minimum function after the previous one, you'll invalidate the maximum function with lastestEntry and change the original behavior of the ComputeTimeSmart function.
In the past achow101 has already purpose a previous PR with this type of approach (#12024). It's an elegant way to solve the issue, but it restrict existing functionality.

This PR solve the issue without removing existing functionality.

Code review ACK bb6bc61. No significant changes since last review other than rebase, optimizing test, and switching to max block time instead of block time.

Thanks for looking into txordered suggestions! It'd be nice to clean up this code more but I can see how listtransaction constrains changing txordered, and I can see upsides of keeping ComputeTimeSmart functional even I might still prefer to see time logic less spread out over two method. There's space for more improvements later but I think this PR is clearly better than status quo.

One thing I don't know is what problem may cause the other reported issue #20591 (comment). Might be worth filing separate bug if there are known steps to reproduce.

Thanks ryanofsky for your feedback and your code review !

For the [#20591 (comment)], I need some confirmation from @charlierlee on my analysis :
I think that @charlierlee encountered an incorrect datation in a wallet.
After that, he searched for a solution online, found this PR and try it with a rescan of the wallet.
His incorrect datation was not corrected after that and he give his feedback here.

My point is that in that case : this PR won't save you because you can't recover an incorrect datation. It's an unrecoverable state. I mention in a previous comment "there's no way to correct transaction datation later : the only way is to destroy the wallet.". The rescan function never change a previous datation (even with a full rescan). That's why I think this PR is important, because it prevent the bad datation to occured in the first place.

For the [#20591 (comment)], @BitcoinTsunami yes that is correct.

@promag did want to follow up on #20591 (comment)?

To me this PR seems like a straightforward improvement that gets the rescan context taken into account in a simple way and adds test coverage. I'm sure there are other ways it could be improved beyond this, but I think this change should only help make future improvements and not get in the way.

Same code. I've just rebased from the latest master code to resolve conflict.

Thanks meshcollider and jonatack for your feedback.
All your recommendations are included in the new code.

ACK 240ea29 per git diff b92d552 240ea29, re-verified rebase to latest master + debug build clean + new test passes on the branch and fails on master, only change since my review a few hours ago is incorporation of latest review suggestions