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

P2P: add maxtimeadjustment command line option #7573

Merged
merged 1 commit into from Mar 30, 2016

Conversation

Projects
None yet
8 participants
@mruddy
Contributor

mruddy commented Feb 21, 2016

Add trustsystemclock command line option to adjust whether peer input can affect the local perspective on time.

I know that there are multiple issues open dealing with local node time management (e.g.- #4521).
I didn't see a pull request though. So, here's one.

This is the same patch set that gained acceptance as being useful for https://github.com/bitcoinxt/bitcoinxt/pull/35/files.

It's a really simple change that gives the user the option of letting other nodes influence it's perspective on time vs. not (a new default).

People have been using this for months without a problem that I'm aware of. At least it has actual usage behind it.
If you think making the default "false", to make the current functionality the default, is the way to go to get this included, then I could make that change to this PR.

At least this will give users an easy option to choose how they want to handle it.
On the command line, just use either -trustsystemclock=0 or -trustsystemclock=1 (the default in this initial proposed commit).

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 21, 2016

Contributor

Concept ACK. This is the "one" way of "Never go to sea with two chronometers; take one or three.".

Yes, please change the default...

No need to call AddTimeData() at all if we trust system clock. Or you can collect time samples and warn user when his system clock is wrong "a lot"...

Contributor

paveljanik commented Feb 21, 2016

Concept ACK. This is the "one" way of "Never go to sea with two chronometers; take one or three.".

Yes, please change the default...

No need to call AddTimeData() at all if we trust system clock. Or you can collect time samples and warn user when his system clock is wrong "a lot"...

@wangchun

This comment has been minimized.

Show comment
Hide comment
@wangchun

wangchun Feb 21, 2016

Concept ACK. But the default value should be set to false.

wangchun commented Feb 21, 2016

Concept ACK. But the default value should be set to false.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 21, 2016

Member

Concept NAK; especially with the default value.

As is, there is almost never any active monitoring it's quite common for systems even with network time configured to have totally incorrect time because the network time has simply silently stopped working. At RWC some persons from the google chrome team presented data collected from chrome users that showed very high rates of time inaccuracy on hosts (and also showed that a significant fraction of all SSL certificate errors are from clients with the wrong date).

IIRC Gnutella has the ability to query the local NTP daemon to determine if it believes that its in sync. It might be more reasonable to have an option to "use local time if and only if the local time server believes that it is in sync".

Of course all this ignores that the normal configuration of NTP is completely insecure, easily spoofed, will believe completely implausible information (e.g. that time is a decade before the release of the software), no effort made against sybil resistance, and there are already known incidents of malicious ntp servers in the public NTP pools... but as a default off option, perhaps that is less of a concern.

Member

gmaxwell commented Feb 21, 2016

Concept NAK; especially with the default value.

As is, there is almost never any active monitoring it's quite common for systems even with network time configured to have totally incorrect time because the network time has simply silently stopped working. At RWC some persons from the google chrome team presented data collected from chrome users that showed very high rates of time inaccuracy on hosts (and also showed that a significant fraction of all SSL certificate errors are from clients with the wrong date).

IIRC Gnutella has the ability to query the local NTP daemon to determine if it believes that its in sync. It might be more reasonable to have an option to "use local time if and only if the local time server believes that it is in sync".

Of course all this ignores that the normal configuration of NTP is completely insecure, easily spoofed, will believe completely implausible information (e.g. that time is a decade before the release of the software), no effort made against sybil resistance, and there are already known incidents of malicious ntp servers in the public NTP pools... but as a default off option, perhaps that is less of a concern.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Feb 21, 2016

Contributor

@paveljanik @wangchun OK, I changed the default to false. I pretty much expected that to be the first thing mentioned, thanks for confirming :) I only left it that way to keep merging easier for anyone that was already using it downstream.

@gmaxwell Thanks for your perspective. That point about clocks causing cert validation errors is interesting. I would not have guessed that. With the default changed to false now, does that make this more acceptable to you? The latest commit basically leaves people in the same boat as they are now, but it gives knowledgeable users that choose to override the ability to use only their local clock.

Contributor

mruddy commented Feb 21, 2016

@paveljanik @wangchun OK, I changed the default to false. I pretty much expected that to be the first thing mentioned, thanks for confirming :) I only left it that way to keep merging easier for anyone that was already using it downstream.

@gmaxwell Thanks for your perspective. That point about clocks causing cert validation errors is interesting. I would not have guessed that. With the default changed to false now, does that make this more acceptable to you? The latest commit basically leaves people in the same boat as they are now, but it gives knowledgeable users that choose to override the ability to use only their local clock.

@jonasschnelli jonasschnelli added the P2P label Feb 22, 2016

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 23, 2016

Contributor

@mruddy Can you lease add warning to the user when his clock is too different to the network time?

Contributor

paveljanik commented Feb 23, 2016

@mruddy Can you lease add warning to the user when his clock is too different to the network time?

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Feb 23, 2016

Contributor

@paveljanik The existing warning will still trigger for that scenario. Please see here https://github.com/mruddy/bitcoin/blob/a1a4d8a5de73e018485ec8fe67d3918b625ed0d0/src/timedata.cpp#L110.
This is because the P2P version message still calls AddTimeData. I didn't change any of that. This change leaves all that old time computation stuff, but ignores the offset that is calculated from it if the new command line switch is set. This is a super simple commit.

Contributor

mruddy commented Feb 23, 2016

@paveljanik The existing warning will still trigger for that scenario. Please see here https://github.com/mruddy/bitcoin/blob/a1a4d8a5de73e018485ec8fe67d3918b625ed0d0/src/timedata.cpp#L110.
This is because the P2P version message still calls AddTimeData. I didn't change any of that. This change leaves all that old time computation stuff, but ignores the offset that is calculated from it if the new command line switch is set. This is a super simple commit.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 23, 2016

Member

@mruddy Have you looked into making it conditional on the local NTP thinking its in sync?

Member

gmaxwell commented Feb 23, 2016

@mruddy Have you looked into making it conditional on the local NTP thinking its in sync?

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Feb 23, 2016

Contributor

@gmaxwell No, but not because I ignored the input.

The first reason is that I don't trust myself to not mess it up and add some kind of vector where a malformed response would allow node takeover etc... The extra attack surface makes me apprehensive.

Second, when I think of how this would be used, the first use-case that comes to mind is of a node operator that notices some other node(s) trying to jack up its perspective on time and (s)he wants to stop that. I envision that admin to be actively monitoring that his/her system time is within reason. The second use case is that of an operator that simply wants to trust the local clock as a standard operating procedure. Again, I envision that user as having made a conscious effort to commit to maintaining the node time through normal process and procedure.

Lastly, in the case where NTP has been spoofed/tampered, or is just not reliable (silent failure etc...), as you mentioned before, then the extra complexity does not seem to help by my way of thinking.

Contributor

mruddy commented Feb 23, 2016

@gmaxwell No, but not because I ignored the input.

The first reason is that I don't trust myself to not mess it up and add some kind of vector where a malformed response would allow node takeover etc... The extra attack surface makes me apprehensive.

Second, when I think of how this would be used, the first use-case that comes to mind is of a node operator that notices some other node(s) trying to jack up its perspective on time and (s)he wants to stop that. I envision that admin to be actively monitoring that his/her system time is within reason. The second use case is that of an operator that simply wants to trust the local clock as a standard operating procedure. Again, I envision that user as having made a conscious effort to commit to maintaining the node time through normal process and procedure.

Lastly, in the case where NTP has been spoofed/tampered, or is just not reliable (silent failure etc...), as you mentioned before, then the extra complexity does not seem to help by my way of thinking.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Feb 23, 2016

Member

Indeed, detection would not help when someone is exploiting NTP's normal lack of security to change the system time; I didn't intend to suggest otherwise. Sorry.

I don't agree with your attack surface concern, after all-- it's all local-- portability may turn out to be more of an issue. But code to harden up this option to throw an error or refuse to operate when set to trust the local clock but the local clock is detectably undisciplined could be done as a separate PR.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

Member

gmaxwell commented Feb 23, 2016

Indeed, detection would not help when someone is exploiting NTP's normal lack of security to change the system time; I didn't intend to suggest otherwise. Sorry.

I don't agree with your attack surface concern, after all-- it's all local-- portability may turn out to be more of an issue. But code to harden up this option to throw an error or refuse to operate when set to trust the local clock but the local clock is detectably undisciplined could be done as a separate PR.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Feb 24, 2016

Contributor

Yep, you're absolutely right that portability might be the devil in the details for the NTP checking. Even being all local, it's still conditional logic that could be triggered under a remote attacker's influence. So, at that point it depends on your threat model whether an attacker being able to influence that exchange is in scope. If that's in scope, then possibly a concern that doesn't depend on a buffer overflow etc... would be where that could be used as a trigger for a previously installed trojan that listens where the NTP listener would have been and then does something bad. That's getting a little far fetched, but it's what came to mind.

I'll have to double check on the performance question and get back to you. I know when I first worked on this about six months ago or so, I did wonder that. We went with it, but I forget if that was just because it was the typical pattern and the code was simple, or if it just did not matter performance-wise.

off topic: Speaking of performance, I went to prune a current datadir today that had txindex on. Of course that caused me to have to reindex. I ran 0.12 to do that and was done soooo much faster thanks to sipa and your excellent work on libsecp256k1. Thank-you!

Contributor

mruddy commented Feb 24, 2016

Yep, you're absolutely right that portability might be the devil in the details for the NTP checking. Even being all local, it's still conditional logic that could be triggered under a remote attacker's influence. So, at that point it depends on your threat model whether an attacker being able to influence that exchange is in scope. If that's in scope, then possibly a concern that doesn't depend on a buffer overflow etc... would be where that could be used as a trigger for a previously installed trojan that listens where the NTP listener would have been and then does something bad. That's getting a little far fetched, but it's what came to mind.

I'll have to double check on the performance question and get back to you. I know when I first worked on this about six months ago or so, I did wonder that. We went with it, but I forget if that was just because it was the typical pattern and the code was simple, or if it just did not matter performance-wise.

off topic: Speaking of performance, I went to prune a current datadir today that had txindex on. Of course that caused me to have to reindex. I ran 0.12 to do that and was done soooo much faster thanks to sipa and your excellent work on libsecp256k1. Thank-you!

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 24, 2016

Member

No problem with adding this, although I don't think it will be used a lot in practice. It should definitely not be enabled by default.

But I understand the use case where you have an expensive trusted time source, and you don't want bitcoind messing with it, even if it means well.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

Yes - the preferred way to do this would be to set a global flag in AppInit2, then use that in the time function. Calling the argument parser every time someone asks for the time is not very elegant, at least.

Or possibly even better: have the option disable the time-offset logic, set the time offset to 0? That'd remove even need to change the GetAdjustedTime() function itself.

Nits:

  • Add a DEFAULT_TRUST_SYSTEM_TIME constant as is done for other options instead of hardcode the default in the help message and GetBoolArg line.
Member

laanwj commented Feb 24, 2016

No problem with adding this, although I don't think it will be used a lot in practice. It should definitely not be enabled by default.

But I understand the use case where you have an expensive trusted time source, and you don't want bitcoind messing with it, even if it means well.

Am I incorrect in my recollection that the getargument stuff is slow? Do we want that inside GetAdjustedTime?

Yes - the preferred way to do this would be to set a global flag in AppInit2, then use that in the time function. Calling the argument parser every time someone asks for the time is not very elegant, at least.

Or possibly even better: have the option disable the time-offset logic, set the time offset to 0? That'd remove even need to change the GetAdjustedTime() function itself.

Nits:

  • Add a DEFAULT_TRUST_SYSTEM_TIME constant as is done for other options instead of hardcode the default in the help message and GetBoolArg line.
@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 24, 2016

Contributor

Idea. This can even be extended. If I trust my time source, I can ban nodes with a huge time difference from my exact time to prevent network partition attacks on me (bad nodes trying to attack me on my node startup with a huge time difference bringing me off the main network). Not that I have seen such attack myself yet, but...

Contributor

paveljanik commented Feb 24, 2016

Idea. This can even be extended. If I trust my time source, I can ban nodes with a huge time difference from my exact time to prevent network partition attacks on me (bad nodes trying to attack me on my node startup with a huge time difference bringing me off the main network). Not that I have seen such attack myself yet, but...

@MarcoFalke

View changes

Show outdated Hide outdated src/init.cpp
@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Feb 25, 2016

Contributor

Thanks, I just pushed the updates mentioned by your feedback.

@laanwj I chose not to disable the time-offset logic entirely because I've gotten feedback about people liking to get the warning message "Please check that your computer's date and time are correct!" if none of the peers are within 5 minutes just in-case.

@paveljanik I also did not add any logic to ban nodes with largely differing times because I did not want to add side-effects that could increase P2P network partitioning. Right now, if the median offset is more than 70 minutes, the offset is set to zero and a warning is logged if none of the nodes are within five minutes. So, there is some protection against an attack that attempts to change your node's perspective on time a lot. If your node disagrees substantially with every other node, then you get a warning. But you'll continue to exchange transactions and blocks etc... If banning them turns out to be desirable, then a separate PR might be the way to go.

Contributor

mruddy commented Feb 25, 2016

Thanks, I just pushed the updates mentioned by your feedback.

@laanwj I chose not to disable the time-offset logic entirely because I've gotten feedback about people liking to get the warning message "Please check that your computer's date and time are correct!" if none of the peers are within 5 minutes just in-case.

@paveljanik I also did not add any logic to ban nodes with largely differing times because I did not want to add side-effects that could increase P2P network partitioning. Right now, if the median offset is more than 70 minutes, the offset is set to zero and a warning is logged if none of the nodes are within five minutes. So, there is some protection against an attack that attempts to change your node's perspective on time a lot. If your node disagrees substantially with every other node, then you get a warning. But you'll continue to exchange transactions and blocks etc... If banning them turns out to be desirable, then a separate PR might be the way to go.

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Feb 25, 2016

Contributor

Please squash into one commit for easier review.

Contributor

paveljanik commented Feb 25, 2016

Please squash into one commit for easier review.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Feb 25, 2016

Contributor

Sure thing, done.

Contributor

mruddy commented Feb 25, 2016

Sure thing, done.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 29, 2016

Member

utACK 1079b2c
This code change is nicely self-contained and localized. Let's not make any changes to the P2P code in this pull, also any proposal regarding automatic banning (for any reason) has to be very carefully evaluated for advantages and dangers, for the reasons already mentioned - partitioning risk.

Member

laanwj commented Feb 29, 2016

utACK 1079b2c
This code change is nicely self-contained and localized. Let's not make any changes to the P2P code in this pull, also any proposal regarding automatic banning (for any reason) has to be very carefully evaluated for advantages and dangers, for the reasons already mentioned - partitioning risk.

@laanwj laanwj added the Feature label Feb 29, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Mar 5, 2016

Member

@gmaxwell I am not convinced that the behaviour of not correcting at all is less harmful than correcting your clock based on what random peers tell you when chosen by someone who has confidence in their own clock configuration.

Member

sipa commented Mar 5, 2016

@gmaxwell I am not convinced that the behaviour of not correcting at all is less harmful than correcting your clock based on what random peers tell you when chosen by someone who has confidence in their own clock configuration.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Mar 5, 2016

Contributor

Perhaps don't think of it as someone "having confidence in their own clock configuration", but rather "wanting to have a consistent or specific perspective on time". For example, maybe a company has nodes in more than one geo location and wants to have a closely consistent perspective on time across all of its nodes. Or, maybe that company wants to have a cluster of nodes running on internally controlled time and a cluster running on network adaptive time. Perhaps there are attacker nodes that feed 69 minute time skews to peers based on user agent and those affected user agents don't want to have their ability to follow consensus affected. From a decentralized validation point of view, I don't see why it would be necessarily bad for a node to take a firm position, or have confidence in its own configuration, on what time it thought it was.

Contributor

mruddy commented Mar 5, 2016

Perhaps don't think of it as someone "having confidence in their own clock configuration", but rather "wanting to have a consistent or specific perspective on time". For example, maybe a company has nodes in more than one geo location and wants to have a closely consistent perspective on time across all of its nodes. Or, maybe that company wants to have a cluster of nodes running on internally controlled time and a cluster running on network adaptive time. Perhaps there are attacker nodes that feed 69 minute time skews to peers based on user agent and those affected user agents don't want to have their ability to follow consensus affected. From a decentralized validation point of view, I don't see why it would be necessarily bad for a node to take a firm position, or have confidence in its own configuration, on what time it thought it was.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 11, 2016

Member

I've thought it over a bit and I still think GetTimeOffset should return 0 when -trustsystemclock is given. This will make the RPCs (getinfo, getnetworkinfo) give the correct effective time offset.

Member

laanwj commented Mar 11, 2016

I've thought it over a bit and I still think GetTimeOffset should return 0 when -trustsystemclock is given. This will make the RPCs (getinfo, getnetworkinfo) give the correct effective time offset.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Mar 13, 2016

Contributor

Sure, that makes sense too. Update made and rebased/squashed.

Contributor

mruddy commented Mar 13, 2016

Sure, that makes sense too. Update made and rebased/squashed.

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Mar 29, 2016

Contributor

This seemed to be lingering, so, here's a commit with a little different approach.
Now, instead of adding -trustsystemclock (that effectively limited the max offset to zero), I added -maxtimeadjustment, that lets the user choose how much of an adjustment (in seconds) is acceptable.
Thus, -trustsystemclock and -maxtimeadjustment=0 give the same basic effect of not letting peers influence the local node's perspective of time.
But, this new way gives a superset of possible choices with less code and probably less runtime overhead because it does not add a condition to GetTimeOffset.

@laanwj @sipa Do you find this any more acceptable? If not, I'll close this and forget about it. No big deal to me either way. Thanks!

Contributor

mruddy commented Mar 29, 2016

This seemed to be lingering, so, here's a commit with a little different approach.
Now, instead of adding -trustsystemclock (that effectively limited the max offset to zero), I added -maxtimeadjustment, that lets the user choose how much of an adjustment (in seconds) is acceptable.
Thus, -trustsystemclock and -maxtimeadjustment=0 give the same basic effect of not letting peers influence the local node's perspective of time.
But, this new way gives a superset of possible choices with less code and probably less runtime overhead because it does not add a condition to GetTimeOffset.

@laanwj @sipa Do you find this any more acceptable? If not, I'll close this and forget about it. No big deal to me either way. Thanks!

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 29, 2016

Member

utACK, I like this better, you've un-magified a magic number (and you can still achieve the same as before, by setting it to 0).

Member

laanwj commented Mar 29, 2016

utACK, I like this better, you've un-magified a magic number (and you can still achieve the same as before, by setting it to 0).

@laanwj

View changes

Show outdated Hide outdated src/util.h

@mruddy mruddy changed the title from P2P: add trustsystemclock command line option to P2P: add maxtimeadjustment command line option Mar 29, 2016

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Mar 29, 2016

Member

I like this approach much better. But the implementation has an odd non-monotonicity. E.g. if you have 5 peers claiming ten seconds off, then six more connect claiming 80 minutes. You'll apply a correction of 0 instead of 10 seconds. Perhaps better to just fix in another PR, since it was an existing misbehavior (though more likely to be triggered when the time is cut down to small numbers).

Member

gmaxwell commented Mar 29, 2016

I like this approach much better. But the implementation has an odd non-monotonicity. E.g. if you have 5 peers claiming ten seconds off, then six more connect claiming 80 minutes. You'll apply a correction of 0 instead of 10 seconds. Perhaps better to just fix in another PR, since it was an existing misbehavior (though more likely to be triggered when the time is cut down to small numbers).

@mruddy

This comment has been minimized.

Show comment
Hide comment
@mruddy

mruddy Mar 29, 2016

Contributor

@gmaxwell Interesting. True, and in that scenario a warning message also gets output when the median goes beyond the acceptable limit.

Maybe you were working towards the, perhaps more alarming, scenario where there are five nodes at 10 seconds off and then six more claim the well-known default acceptable limit of 4200 seconds.
That would cause a large effective change in offset with no other warning.
Maybe that could be the basis for another warning message (in a separate PR)?
Basically, the offset is within tolerance, but there might be something going on.
We'd need to add an option to set the threshold of change to alert that there might be "something going on".
This is different than simply seeing a large range between min and max offset seen because it actually changes our local offset.

Now that this value no longer a hard-coded constant, a possible mitigation would be to unpredictably pick a value between 0 and 4200 so that attacker nodes would run a higher risk of sending back offsets that are out of bounds.

Also, in your scenario, I'm not sure which set of nodes are attacker nodes (or maybe they're two sets of nodes working at cross-purposes).

For example, this is what's really on my mind:

I've been working on a CLTV-based unidirectional payment channel vulnerability assessment and two actual related threats that come to mind from that are:

  • miners that want to get fees from time locked transactions by putting them in blocks "early" (will be mitigated by BIP113)
  • senders that want to claim the channel's reserve value by getting a refund transaction mined before the receiver can spend a transaction allocating value to her

Both threats gain advantage by having more nodes accept into their mempool and relay time-locked transactions early.
Even with BIP113, if a sender can get a non-opt-in-RBF sequence value containing transaction propagated early, that inhibits the receiver's ability to even propagate an honest allocation transaction later.

So maybe the bad actors set up a bunch of passive nodes that, when connected to, return valid high time offsets.

Contributor

mruddy commented Mar 29, 2016

@gmaxwell Interesting. True, and in that scenario a warning message also gets output when the median goes beyond the acceptable limit.

Maybe you were working towards the, perhaps more alarming, scenario where there are five nodes at 10 seconds off and then six more claim the well-known default acceptable limit of 4200 seconds.
That would cause a large effective change in offset with no other warning.
Maybe that could be the basis for another warning message (in a separate PR)?
Basically, the offset is within tolerance, but there might be something going on.
We'd need to add an option to set the threshold of change to alert that there might be "something going on".
This is different than simply seeing a large range between min and max offset seen because it actually changes our local offset.

Now that this value no longer a hard-coded constant, a possible mitigation would be to unpredictably pick a value between 0 and 4200 so that attacker nodes would run a higher risk of sending back offsets that are out of bounds.

Also, in your scenario, I'm not sure which set of nodes are attacker nodes (or maybe they're two sets of nodes working at cross-purposes).

For example, this is what's really on my mind:

I've been working on a CLTV-based unidirectional payment channel vulnerability assessment and two actual related threats that come to mind from that are:

  • miners that want to get fees from time locked transactions by putting them in blocks "early" (will be mitigated by BIP113)
  • senders that want to claim the channel's reserve value by getting a refund transaction mined before the receiver can spend a transaction allocating value to her

Both threats gain advantage by having more nodes accept into their mempool and relay time-locked transactions early.
Even with BIP113, if a sender can get a non-opt-in-RBF sequence value containing transaction propagated early, that inhibits the receiver's ability to even propagate an honest allocation transaction later.

So maybe the bad actors set up a bunch of passive nodes that, when connected to, return valid high time offsets.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 30, 2016

Member

Agreed, let's fix that to another pull.

There is no obligation for @mruddy to fix the peer time synchronization system, which is a herculean task, especially as it seems he doesn't want to use it at all. See #4521 for a collection of peer time related issues, will link there to this one too.

Member

laanwj commented Mar 30, 2016

Agreed, let's fix that to another pull.

There is no obligation for @mruddy to fix the peer time synchronization system, which is a herculean task, especially as it seems he doesn't want to use it at all. See #4521 for a collection of peer time related issues, will link there to this one too.

@laanwj laanwj merged commit e1523ce into bitcoin:master Mar 30, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 30, 2016

Merge #7573: P2P: add maxtimeadjustment command line option
e1523ce P2P: add maxtimeadjustment command line option (mruddy)

@laanwj laanwj referenced this pull request Mar 30, 2016

Closed

TODO for release notes 0.13.0 #7678

14 of 16 tasks complete

@mruddy mruddy deleted the mruddy:trust-system-clock branch Mar 30, 2016

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7573: P2P: add maxtimeadjustment command line option
e1523ce P2P: add maxtimeadjustment command line option (mruddy)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7573: P2P: add maxtimeadjustment command line option
e1523ce P2P: add maxtimeadjustment command line option (mruddy)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7573: P2P: add maxtimeadjustment command line option
e1523ce P2P: add maxtimeadjustment command line option (mruddy)

codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge #7573: P2P: add maxtimeadjustment command line option
e1523ce P2P: add maxtimeadjustment command line option (mruddy)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment