Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use recent block times for unlock time #6745

Merged
merged 2 commits into from Sep 16, 2020

Conversation

moneromooo-monero
Copy link
Collaborator

@moneromooo-monero moneromooo-monero commented Aug 5, 2020

From an original patch by TheCharlatan.

@@ -3682,7 +3684,7 @@ bool Blockchain::is_tx_spendtime_unlocked(uint64_t unlock_time) const
else
{
//interpret as time
uint64_t current_time = static_cast<uint64_t>(time(NULL));
const uint64_t current_time = hf_version >= 13 ? get_adjusted_time(m_db->height()) : time(NULL);
Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is removing the static cast for time(NULL) ok/intended here?

Also, according to the other changes in this PR get_adjusted_time() will return the median of the last 60 blocks' timestamps.. which should be 30 blocks ago (60 minutes ago) on average. Not the current time. Am I missing something or this broken?

One way to fix this is to estimate the current time from the median block's time.

//approximate the time (in UTC) of the current block based on the block that should be positioned at 1/2 of the blockchain check window, and how often blocks are produced on average
//using the median means the 'height' block author can't manipulate their timestamp to significantly affect the return value from this function
... get_adjusted_time(m_db->height()) + static_cast<uint64_t>((BLOCKCHAIN_TIMESTAMP_CHECK_WINDOW/2) * (get_ideal_hard_fork_version(height) < 2 ? DIFFICULTY_TARGET_V1 : DIFFICULTY_TARGET_V2)) ...

Honestly it's a bit messy, so maybe add a function. The terminology is really mixed up here since the behavior of get_adjusted_time() should really be in a function called get_timestamp_check_median_time(), and get_adjusted_time() should return the approximate time of the current block and call get_timestamp_check_median_time() internally.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't recall. It seems pointless anyway.
The whole point is to start using blockchain timestamps instead of local time. This is intended. A 30 minute delay is fine, since this is typically used to lock things long term, not for a few minutes I think ? Define broken. Adding an offset would need changing some tests IIRC, and means you might unlck too early instead of too late. Why do you think this may be needed ?

Copy link
Contributor

@UkoeHB UkoeHB Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By 'broken' I meant 'unintended consequences'. It sounds like the changes are intentional.

"A 30 minute delay is fine, since this is typically used to lock things long term, not for a few minutes I think ?" Presumably if someone is locking a transaction with UTC time, they are trying to be more precise than a block height lock could provide. As the PR stands, users will have to wait an extra 60mins on average for their outputs to be unlocked (relative to the unlock time).

It sounds like you are concerned about future times being used for timelock checks. What about correcting the offset by doing a minimum of the current block's block time and the median block's time + 60mins? If blocks are accelerating then the current block's time will be more accurate than the median + 60, if the current block has a future time then the median + 60 should be closer to the real time, and if blocks are decelerating then authors will have to wait a few extra minutes to spend their outputs.

Note that even without the offset someone who can manipulate the median could push it into the future up to +1hr (since block times are limited by local_time + 2hr according to check_block_timestamp()). Adding the corrected offset allows timelocked tx to be spent up to 2hrs early in the worst case, while being much closer to +/- 10mins in the typical case.

Copy link
Contributor

@TheCharlatan TheCharlatan Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concern goes in both directions, faster and slower blocks, meaning both block timestamps into the future and the past. I'd argue a window of +-60 minutes is the time precission that we can provide and that it is good enough. If you lock a transaction's outputs with unix time exactly a year into the future, you get a guaranteed precision of +-60 minutes, while if you use blocks, the precision is unknown and could vary days.

Having the ability to push the median significantly would probably be at the limit of our security boundaries, since it is miners that are pushing it. I have not done any statistics on this though.

Copy link
Contributor

@UkoeHB UkoeHB Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, rather than 'current block' for corrected offset it should be 'previous block + 2mins' since the current block may be under construction and we don't want to use local time.

Well, +/- 60mins might be ok, but I'm wondering why not push for even more precision if it's possible?

Copy link
Contributor

@TheCharlatan TheCharlatan Sep 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a bit of time going through some of the potential problems with koe's proposal and think it is safe enough to go with it. It does introduce some extra complexity though and detecting a few of the corner cases has been challenging. The proposal does allow a miner to block inclusion of a time locked transaction into the next block by setting a low timestamp of his own mined block. In the current protocol, there are no incentives or rewards gained by doing this. Given this added complexity I would like to get a bit more review here.

Copy link
Contributor

@UkoeHB UkoeHB Sep 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To get the adjusted time I think it should be like this:

// project the median to match approximately when the block being validated will appear
// the median is calculated from a chunk of past blocks, so we use +1 to project onto the current block
median_ts += (BLOCKCHAIN_TIMESTAMP_CHECK_WINDOW + 1) * DIFFICULTY_TARGET_V2 / 2;

// project the current block's time based on the previous block's time
// we don't use the current block's time directly to mitigate timestamp manipulation
uint64_t adjusted_current_block_ts = m_db->get_block_timestamp(height - 1) + DIFFICULTY_TARGET_V2;

// return minimum of ~current block time and adjusted median time
// we do this since it's better to report a time in the past than a time in the future
return (adjusted_current_block_ts < median_ts ? adjusted_current_block_ts : median_ts);

Note: on the odd looking +1 in median_ts... imagine:

  1. window = 2, blocks are indexed b0, b1, b2; block b2 is being validated; median of window is at timestamp (b0 + b1)/2 = 0.5b; adding offset of window/2 means adjusted timestamp would be 1.5b so we need to add another 0.5b -> 0.5b + (window + 1)/2 = 2b
  2. window = 3, blocks are indexed b0, b1, b2, b3; block b3 is being validated; median of window is at timestamp 1b; adding offset of window/2 means adjusted timestamp would be 2.5b so we need to add another 0.5b -> 1b + (window + 1)/2 = 3b

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume adjusted_current_block_ts and previous_block_ts are the same variable here, right ?

Copy link
Contributor

@UkoeHB UkoeHB Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah fixed

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pushed with that change.

if(b.timestamp > (uint64_t)time(NULL) + CRYPTONOTE_BLOCK_FUTURE_TIME_LIMIT)
{
MERROR_VER("Timestamp of block with id: " << get_block_hash(b) << ", " << b.timestamp << ", bigger than adjusted time + 2 hours");
Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(b.timestamp > (uint64_t)time(NULL) + CRYPTONOTE_BLOCK_FUTURE_TIME_LIMIT)
{
MERROR_VER("Timestamp of block with id: " << get_block_hash(b) << ", " << b.timestamp << ", bigger than adjusted time + 2 hours");
if(b.timestamp > (uint64_t)time(NULL) + CRYPTONOTE_BLOCK_FUTURE_TIME_LIMIT)
{
MERROR_VER("Timestamp of block with id: " << get_block_hash(b) << ", " << b.timestamp << ", bigger than local time + 2 hours");

/**
* @brief get the "adjusted time"
*
* Currently this simply returns the current time according to the
* user's machine.
*
* @return the current time
*/
uint64_t get_adjusted_time() const;

Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this declaration moved up 400 lines? New spot seems less appropriate based on other declarations nearby.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To make it public.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess I could add public: and private: and keep it where it was though. Seemed a bit meh but it minimizes the diff...

Copy link
Contributor

@UkoeHB UkoeHB Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah if it's being made public that makes sense.

const uint64_t adjusted_time = m_wallet->get_daemon_adjusted_time();
uint64_t threshold = adjusted_time + (m_wallet->use_fork_rules(2, 0) ? CRYPTONOTE_LOCKED_TX_ALLOWED_DELTA_SECONDS_V2 : CRYPTONOTE_LOCKED_TX_ALLOWED_DELTA_SECONDS_V1);
Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like terminology confusion again, since 'adjusted_time' has been changed to mean 'median time of last 60 blocks'. Which makes the behavior here change.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it was already broken. I'm not sure why it's adding that define. The threshold really should be adjusted_time. If you can't think of why the define gets added either, I'll remove.

Copy link
Contributor

@UkoeHB UkoeHB Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you are referring to. Define?

Copy link
Contributor

@TheCharlatan TheCharlatan Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename it to locktime_median_time_past or something along those lines to avoid confusion.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By define, I meant adding CRYPTONOTE_LOCKED_TX_ALLOWED_DELTA_SECONDS_V2 etc. I don't see what purpose this has.

Copy link
Contributor

@UkoeHB UkoeHB Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a Chesterton's fence? It does feel like an idea that didn't get fully fleshed out. It has been there ever since the original fork.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, it doesn't hurt anything either. So as you imply, safer to keep it in at least for now.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name seems clear to me. It's getting the adjusted time from the daemon, not the current time.

}

return check_block_timestamp(timestamps, b, median_ts);
Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this 3-parameter overload of check_block_timestamp() being deprecated? If so, might be nice to remove it completely.

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's still used.

const uint64_t adjusted_time = m_wallet->get_daemon_adjusted_time();
uint64_t threshold = adjusted_time + (m_wallet->use_fork_rules(2, 0) ? CRYPTONOTE_LOCKED_TX_ALLOWED_DELTA_SECONDS_V2 : CRYPTONOTE_LOCKED_TX_ALLOWED_DELTA_SECONDS_V1);
Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the same as the one above. I think I might be missing your point though, since the point of the patch is to change the behaviour. So can you expand on whether you think something else (than the addition) is wrong ?

Copy link
Contributor

@UkoeHB UkoeHB Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, just pointing out that every use of 'adjusted_time' is really '60 mins ago on average'. Since they are all replacing 'current time' the behavior changes.

@@ -5932,7 +5932,7 @@ uint64_t wallet2::balance(uint32_t index_major, bool strict) const
return amount;
}
//----------------------------------------------------------------------------------------------------
uint64_t wallet2::unlocked_balance(uint32_t index_major, bool strict, uint64_t *blocks_to_unlock, uint64_t *time_to_unlock) const
uint64_t wallet2::unlocked_balance(uint32_t index_major, bool strict, uint64_t *blocks_to_unlock, uint64_t *time_to_unlock)
Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was const removed from all of these functions?

Copy link
Collaborator Author

@moneromooo-monero moneromooo-monero Aug 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because they now call RPC.

// XXX: this needs to be fast, so we'd need to get the starting heights
// from the daemon to be correct once voting kicks in
uint64_t v2height = m_nettype == TESTNET ? 624634 : m_nettype == STAGENET ? 32000 : 1009827;
uint64_t leeway = block_height < v2height ? CRYPTONOTE_LOCKED_TX_ALLOWED_DELTA_SECONDS_V1 : CRYPTONOTE_LOCKED_TX_ALLOWED_DELTA_SECONDS_V2;
if(current_time + leeway >= unlock_time)
if(adjusted_time + leeway >= unlock_time)
Copy link
Contributor

@UkoeHB UkoeHB Aug 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change

@selsta
Copy link
Collaborator

selsta commented Aug 7, 2020

Is the unbound submodule update intended?

@moneromooo-monero
Copy link
Collaborator Author

moneromooo-monero commented Aug 8, 2020

No. Not sure how it ended up here, thanks.

@moneromooo-monero
Copy link
Collaborator Author

moneromooo-monero commented Aug 8, 2020

Ah, I found the behaviour change, a place is changing this without checking for v13. Will fix.

@@ -3752,11 +3755,28 @@ bool Blockchain::check_tx_input(size_t tx_version, const txin_to_key& txin, cons
}
//------------------------------------------------------------------
//TODO: Is this intended to do something else? Need to look into the todo there.

This comment has been hidden.

@dEBRUYNE-1
Copy link
Contributor

dEBRUYNE-1 commented Aug 28, 2020

Fyi, has conflicts with src/cryptonote_config.h as far as I can see.

@moneromooo-monero
Copy link
Collaborator Author

moneromooo-monero commented Sep 9, 2020

Now with the expected extra time. I'll rebase later since that'll pollute the diff massively.

@moneromooo-monero
Copy link
Collaborator Author

moneromooo-monero commented Sep 9, 2020

I do not have the space to sync another chain here, it would be nice if someone could try a full sync to make sure this does not break historical blocks :)

@moneromooo-monero
Copy link
Collaborator Author

moneromooo-monero commented Sep 10, 2020

I'm wondering whether this change can lock an ouput after it was unlocked, if the adjusted time goes back in time (ie, a new block has an earlier ts than the one 60 block ago). That could be annoying.

@TheCharlatan
Copy link
Contributor

TheCharlatan commented Sep 10, 2020

If the last block is re-orged then sure, this can happen. However this is even true if you take just the median block times without additional modification. As long as the time used for the validation is relative to whatever the state is when the transaction is first included in a block, a change of the time into the past in a later block does not matter.

/**
* @brief get the "adjusted time"
*
* Returns the median timestamp of the past 60 blocks, if enough blocks are
* available. Returns the current time on the machine, if less than 60
* blocks are available.
*
* @return median time
*/
uint64_t get_adjusted_time(uint64_t height) const;
Copy link
Contributor

@UkoeHB UkoeHB Sep 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is out of date, otherwise everything looks good.

     /**
     * @brief get the "adjusted time"
     *
     * Computes the median timestamp of the previous 60 blocks, projects it
     * onto the current block to get an 'adjusted median time' which approximates
     * what the current block's timestamp should be. Also projects the previous
     * block's timestamp to estimate the current block's timestamp.
     * 
     * Returns the minimum of the two projections, or the current local time on
     * the machine if less than 60 blocks are available.
     *
     * @return current time approximated from chain data
     */

UkoeHB
UkoeHB approved these changes Sep 13, 2020
selsta
selsta approved these changes Sep 14, 2020

// project the current block's time based on the previous block's time
// we don't use the current block's time directly to mitigate timestamp manipulation
uint64_t adjusted_current_block_ts = m_db->get_block_timestamp(height - 1) + DIFFICULTY_TARGET_V2;
Copy link
Contributor

@xiphon xiphon Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint64_t adjusted_current_block_ts = m_db->get_block_timestamp(height - 1) + DIFFICULTY_TARGET_V2;
uint64_t adjusted_current_block_ts = timestamps.back() + DIFFICULTY_TARGET_V2;

selsta
selsta approved these changes Sep 15, 2020
UkoeHB
UkoeHB approved these changes Sep 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants