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 commented at 12:03 PM on December 2, 2021: member

   <!--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.

5. 
   theStack commented at 2:25 PM on December 2, 2021: member

   Concept ACK

6. 
   brunoerg commented at 2:03 PM on December 4, 2021: member

   Concept ACK

7. theStack approved
8. 
   theStack commented at 3:56 PM on December 4, 2021: member

   Code-review ACK fa37e798b2660d8e44e31c944a257b55aeef5de2

9. fanquake requested review from instagibbs on Dec 7, 2021
10. fanquake requested review from achow101 on Dec 7, 2021
11. instagibbs approved
12. 
    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

13. shaavan approved
14. 
    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!

15. MarcoFalke merged this on Dec 7, 2021
16. MarcoFalke closed this on Dec 7, 2021

    ---

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

    Should [part of] this be backported?

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

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

21. luke-jr referenced this in commit 56cd85dfc9 on Dec 14, 2021
22. RandyMcMillan referenced this in commit b1da0b2b48 on Dec 23, 2021
23. DrahtBot locked this on Dec 9, 2022

Review Requested
achow101