wallet: Replace confusing getAdjustedTime() with GetTime() #23644

1. 
   MarcoFalke commented at 3:36 PM on December 1, 2021: member

   Setting nTimeReceived to the adjusted time has several issues:

   • m_best_block_time is set to the "unadjusted" time, thus a comparison of the two times is like comparing apples to oranges. In the worst case this opens up an attack vector where remote peers can force a premature re-broadcast of wallet txs.
   • The RPC documentation for "timereceived" doesn't mention that the network adjusted time is used, possibly confusing users when the time reported by RPC is off by a few seconds compared to their local timestamp.

   Fix all issues by replacing the call with GetTime(). Also a style fix: Use non-narrowing integer conversion in the RPC method.

2. wallet: Replace confusing getAdjustedTime() with GetTime() fa37e798b2
3. MarcoFalke added the label Wallet on Dec 1, 2021
4. DrahtBot cross-referenced this on Dec 2, 2021 from issue MOVEONLY: Move wallet backup and encryption RPCs out of rpcwallet by meshcollider
5. 
   DrahtBot commented at 12:03 PM on December 2, 2021: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #23667 (Split up rpcwallet by meshcollider)

   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.

6. 
   theStack commented at 2:25 PM on December 2, 2021: contributor

   Concept ACK

7. DrahtBot cross-referenced this on Dec 4, 2021 from issue Split up rpcwallet by meshcollider
8. 
   brunoerg commented at 2:03 PM on December 4, 2021: contributor

   Concept ACK

9. theStack approved
10. 
    theStack commented at 3:56 PM on December 4, 2021: contributor

    Code-review ACK fa37e798b2660d8e44e31c944a257b55aeef5de2

11. fanquake requested review from instagibbs on Dec 7, 2021
12. fanquake requested review from achow101 on Dec 7, 2021
13. instagibbs approved
14. 
    instagibbs commented at 2:21 AM on December 7, 2021: member

    Just noting that IIUC unconfirmed transactions' time field will also be changed by this, as it returns the wtx.nTimeReceived value in ComputeTimeSmart. Seems better to trust local clock for local data.

    ACK

15. shaavan approved
16. 
    shaavan commented at 7:37 AM on December 7, 2021: contributor

    crACK fa37e798b2660d8e44e31c944a257b55aeef5de2

    Seems better to trust local clock for local data.

    +1 to this!

17. MarcoFalke merged this on Dec 7, 2021
18. MarcoFalke closed this on Dec 7, 2021

    ---

19. MarcoFalke deleted the branch on Dec 7, 2021
20. sidhujag referenced this in commit 3ad7728397 on Dec 7, 2021
21. 
    luke-jr commented at 1:58 AM on December 9, 2021: member

    Should [part of] this be backported?

22. 
    MarcoFalke commented at 8:26 AM on December 9, 2021: member

    Yes, it can be backported, but doesn't have to.

23. luke-jr referenced this in commit 56cd85dfc9 on Dec 14, 2021
24. RandyMcMillan referenced this in commit b1da0b2b48 on Dec 23, 2021
25. bitcoin locked this on Dec 9, 2022

Review Requested
achow101