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.

Sign up for GitHub

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

Jump to bottom

[PoS] Time Protocol v2 #1002

Merged
merged 26 commits into from Nov 22, 2019
Merged

[PoS] Time Protocol v2 #1002

merged 26 commits into from Nov 22, 2019

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Sep 10, 2019 •
edited

Main changes:

  • granularity: 15 secs, instead of 1
  • future limit: adjustedTime + 14 secs (current timeslot) instead of 3 mins
  • past limit: prevBlock time
  • target timespan: 15 mins (instead of 40)

@random-zebra random-zebra added this to the 4.0.0 milestone Sep 10, 2019
@random-zebra random-zebra self-assigned this Sep 10, 2019
@random-zebra random-zebra added Consensus Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes Protocol-Update and removed Backport Block Storage Brainstorming backlog labels Sep 10, 2019
@furszy furszy self-requested a review September 11, 2019 00:13
@furszy
Copy link

furszy commented Sep 11, 2019

Great PR 👍 , there we go again.

Warrows
Warrows reviewed Sep 11, 2019
View reviewed changes
Copy link

@Warrows Warrows left a comment

Choose a reason for hiding this comment

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

Concept ACK. Great changes in principle. However, I'm wondering if we really need to change the block time.
I know that in terms of code and performance it's good to use a 0xf mask. However changing the block time from 60 to 64 seconds is going to warrant other changes in the budget and emission systems we might want to avoid.
It's also probably going to affect user perception. People won't be able to grasp it comfortably. They are used to certain measures of time, 60 seconds blocks are practical. For any time computation in the daily life system 64 seconds are not so practical.
It think it's doable and relatively easy to keep a 60 seconds block time while keeping the principle of these changes.

src/kernel.cpp Outdated Show resolved Hide resolved
src/chainparams.cpp Show resolved Hide resolved
src/pow.cpp Show resolved Hide resolved
@random-zebra
Copy link
Author

So @Warrows you are basically suggesting to use a granularity of 15 (instead of 16) in order to keep the blocktime at 60.
I don't love to give up on the bitmask's performance in favor of the integer division operation, but the benefits you outline are tangible. Will code the changes and we'll see on testnet how it performs.
(will also use slightly different names as the time would not be "masked" anymore but rather subdivided in slots, each one having a duration of 15 seconds).

@Warrows Warrows self-requested a review September 13, 2019 10:27
@random-zebra random-zebra force-pushed the 2019_time_protocol_V2 branch 2 times, most recently from d86ede2 to f892c7c Compare September 23, 2019 01:34
furszy
furszy previously approved these changes Sep 27, 2019
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

After segregated testnet testing, all looks good. Just minor adjustments that needs to be done in the difficulty algorithm that can be achieved in a subsequent PR for me.

ACK f892c7c

Fuzzbawls
Fuzzbawls previously approved these changes Sep 27, 2019
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK f892c7c

Warrows
Warrows previously approved these changes Sep 28, 2019
View reviewed changes
Copy link

@Warrows Warrows left a comment

Choose a reason for hiding this comment

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

ACK f892c7c

random-zebra and others added 15 commits November 20, 2019 23:22
@random-zebra
[PoS] set nTargetTimespan_V2 to 15 minutes (60 time slots)
fee5f5c
@random-zebra
[Core] Guard the transition to v2 time protocol in MinPastBlockTime rule
ec45670
@random-zebra
[PoS] Keep v1 miner until hard-fork
0bcf0c1
@random-zebra
[Core] Make bnProofOfStakeLimit_V2 16 times v1 limit
3447dfb
@random-zebra
[PoS] reduce difficulty by a factor 16 on nBlockTimeProtocolV2
59cd3af 
instead of resetting it to the limit
@random-zebra
[PoS] StakeV1: iterate the time backwards.
c97e998 
makes the miner more efficient, reducing the amount of kernels re-hashed
between successive runs.
@random-zebra
[PoS] Don't allow consecutive blocks within the same time slot
4afcba5
@furszy @random-zebra
[Performance] Kernel, methods using object reference instead of copy …
55faec1 
…the object.
@random-zebra
[PoS] Don't use chainActive.Tip() as prevIndex when creating new blocks
3bb0c0a 
use mapBlockIndex instead
@random-zebra
[Core] Restore CheckBlock checks before previous block check
79bdc83 
in ProcessNewBlock (as it was before nStakeModifierV2) to avoid possible
syncing issues.
@random-zebra
[Core] raise nTargetTimespan_V2 to 30 minutes
55aeecd
@random-zebra
[PoS] Fix Stake maturity-check and mapHashedBlocks time
3487e58
@random-zebra
[Tests] (squashed) few regtest fixes
8ca3979 
- fix regtest nStakeMinAge check
- fix zerocoin_public_spend
- fix StakeModifier V2 generation on regtest (0)
- fix stake age on regtest
@random-zebra
[P2P] Do not store more than 200 timedata samples.
c254df6 
Backport of bitcoin#6545
@random-zebra
[Consensus] Fix difficulty adjustment on first block of timeproto V2
4ea0476
furszy
furszy approved these changes Nov 21, 2019
View reviewed changes
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Great, ACK 4ea0476

Mrs-X
Mrs-X reviewed Nov 21, 2019
View reviewed changes
src/main.cpp Show resolved Hide resolved
Mrs-X
Mrs-X approved these changes Nov 22, 2019
View reviewed changes
Copy link

@Mrs-X Mrs-X left a comment

Choose a reason for hiding this comment

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

ACK 4ea0476

furszy added a commit that referenced this pull request Nov 22, 2019
@furszy
Merge #1002: [PoS] Time Protocol v2
5666184 
4ea0476 [Consensus] Fix difficulty adjustment on first block of timeproto V2 (random-zebra)
c254df6 [P2P] Do not store more than 200 timedata samples. (random-zebra)
8ca3979 [Tests] (squashed) few regtest fixes (random-zebra)
3487e58 [PoS] Fix Stake maturity-check and mapHashedBlocks time (random-zebra)
55aeecd [Core] raise nTargetTimespan_V2 to 30 minutes (random-zebra)
79bdc83 [Core] Restore CheckBlock checks before previous block check (random-zebra)
3bb0c0a [PoS] Don't use chainActive.Tip() as prevIndex when creating new blocks (random-zebra)
55faec1 [Performance] Kernel, methods using object reference instead of copy the object. (furszy)
4afcba5 [PoS] Don't allow consecutive blocks within the same time slot (random-zebra)
c97e998 [PoS] StakeV1: iterate the time backwards. (random-zebra)
59cd3af [PoS] reduce difficulty by a factor 16 on nBlockTimeProtocolV2 (random-zebra)
3447dfb [Core] Make bnProofOfStakeLimit_V2 16 times v1 limit (random-zebra)
0bcf0c1 [PoS] Keep v1 miner until hard-fork (random-zebra)
ec45670 [Core] Guard the transition to v2 time protocol in MinPastBlockTime rule (random-zebra)
fee5f5c [PoS] set nTargetTimespan_V2 to 15 minutes (60 time slots) (random-zebra)
ca2b035 [PoS] Set target spacing to 60 seconds (random-zebra)
0578b0a [PoS] Adjust FutureBlockTimeDrift to 15 secs granularity (random-zebra)
dd71075 [Core] Set time granularity to 15 (instead of 16) (random-zebra)
8c3546f [Consensus] Change difficulty computation for V2 time protocol (random-zebra)
f892e45 [Core/PoS] Enforce timestamp mask in miner (random-zebra)
cdaf818 [Core] Define new Past Time Limit and enforce masked time (random-zebra)
bcb5836 [Core] Define new Future Time Drift (random-zebra)
159eda9 [Cleanup] Remove unused function CheckCoinStakeTimestamp (random-zebra)
9773c09 [Core] Add timestamp mask to chainparams (random-zebra)
1d8fb33 [COPY] Update missing headers (random-zebra)
6f09121 [Consensus] Add placeholder block height for Time Protocol V2 (random-zebra)

Pull request description:

  Main changes:

  - **granularity**: 15 secs, instead of 1
  - **future limit**: adjustedTime + 14 secs (current timeslot) instead of 3 mins
  - **past limit**: prevBlock time
  - **target timespan**: 15 mins (instead of 40)

ACKs for top commit:
  furszy:
    Great, ACK 4ea0476
  Mrs-X:
    ACK 4ea0476

Tree-SHA512: a365297ba169f78ad1c30cc2f318dce09d6e98038a43d35c645814923829e793147e7fdbcbb3f701878a1f5f4892f468f19451fcacc25973db63c92c02e40aa1
@furszy furszy merged commit 4ea0476 into PIVX-Project:master Nov 22, 2019
@random-zebra random-zebra mentioned this pull request Nov 22, 2019
Merged
Warrows
Warrows reviewed Nov 23, 2019
View reviewed changes
Copy link

@Warrows Warrows left a comment

Choose a reason for hiding this comment

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

A bit after the fact but ACK 4ea0476

Fuzzbawls added a commit that referenced this pull request Nov 25, 2019
@Fuzzbawls
Merge #1128: [Consensus] nTimeOffset warning for time protocol V2
d6573e7 
f14af4f [Trivial] Fix timedata.cpp includes (random-zebra)
b6992ac [Tests] Add p2p_time_offset functional test to test_runner (random-zebra)
faf03a6 [Tests] Define p2p_time_offset functional test (random-zebra)
d9e9284 [Consensus] reduce possible nTimeOffset to 15 seconds (random-zebra)

Pull request description:

  Open for brainstorming.
  Follow up to #1002

  - reject connection with a node having offset higher than 30 secs (and don't add the sample to timedata)
  - repeatedly trigger the warning message when the total offset (absolute value) is above 15 seconds (and set the offset to +15/-15 instead of 0).

ACKs for top commit:
  Fuzzbawls:
    utACK f14af4f
  Mrs-X:
    NIT that the last LogPrintf() does not have a time-unit anymore, but utACK f14af4f
  Warrows:
    ACK f14af4f

Tree-SHA512: e2a8b7ee7718814ce49fcb536af48afcf1b2829644699abbe5279b064efe1ff066c3100e65229abf06a7787b9f6e813b971300404841a0e6fe41375780a5e318
@Fuzzbawls Fuzzbawls removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Dec 14, 2019
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 25, 2020
@random-zebra
[Bug] Fix nTimeSmart computation
3ab5e6b 
Given the strict timestamp checks enforced with POS TimeProtocol v2
(introduced with PIVX-Project#1002: a block cannot have a timestamp earlier than
previous blocks and cannot be more than 15 seconds in the future), we
don't need the complex (and error-prone) logic of ComputeTimeSmart. We
can, instead, simply rely on the blocktime, which already ensures a
proper ordering for the transactions.
@random-zebra random-zebra mentioned this pull request Apr 25, 2020
Merged
random-zebra added a commit to random-zebra/PIVX that referenced this pull request Apr 25, 2020
@random-zebra
[Bug] Fix nTimeSmart computation
3f31b4f 
Given the strict timestamp checks enforced with POS TimeProtocol v2
(introduced with PIVX-Project#1002: a block cannot have a timestamp earlier than
previous blocks and cannot be more than 15 seconds in the future), we
don't need the complex (and error-prone) logic of ComputeTimeSmart. We
can, instead, simply rely on the blocktime, which already ensures a
proper ordering for the transactions.
Fuzzbawls added a commit that referenced this pull request Apr 28, 2020
@Fuzzbawls
Merge #1565: [Bug] Simpler nTimeSmart computation
7fb724f 
0c581fb [GUI] Update the record time when wtx.nTimeSmart changes (random-zebra)
b1dc5d3 [Refactor] Encapsulate ComputeTimeSmart in CWalletTx (random-zebra)
db99c41 [Cleanup] Remove unneeded GetComputedTxTime (random-zebra)
3f31b4f [Bug] Fix nTimeSmart computation (random-zebra)

Pull request description:

  Given the strict timestamp checks enforced with POS TimeProtocol v2 (introduced with #1002: a block cannot have a timestamp earlier than previous blocks and cannot be more than 15 seconds in the future), we don't need the complex (and error-prone) logic of the current implementation of `ComputeTimeSmart`.
  We can, instead, simply rely on the blocktime, which already ensures a proper ordering for the transactions.
  This also allows us to encapsulate the function in the `CWalletTx` class (renaming it to `UpdateTimeSmart`).

  We can also get rid of the additional `GetComputedTxTime` (with its additional LOCK, and main chain lookup), and just use `GetTxTime`.

  Finally, update the nSmartTime when a mempool transaction is added to a block.

  This should, hopefully, fix #1346 (further testing needed).

ACKs for top commit:
  furszy:
    ACK 0c581fb .
  Fuzzbawls:
    ACK 0c581fb

Tree-SHA512: 6d2bed1fafa7a658bd42ff2f7fe003087de368e2f49088104bce2409e4370caee0e1a153dc519525d7647b7afa1322fa1be968eefbd299404e1c8a521e24023e
Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request May 5, 2020
@random-zebra @Fuzzbawls
[Bug] Fix nTimeSmart computation
7968c6d 
Given the strict timestamp checks enforced with POS TimeProtocol v2
(introduced with PIVX-Project#1002: a block cannot have a timestamp earlier than
previous blocks and cannot be more than 15 seconds in the future), we
don't need the complex (and error-prone) logic of ComputeTimeSmart. We
can, instead, simply rely on the blocktime, which already ensures a
proper ordering for the transactions.
Github-Pull: PIVX-Project#1565
Rebased-From: 3f31b4f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Warrows Warrows Warrows left review comments

@furszy furszy furszy approved these changes

@Mrs-X Mrs-X Mrs-X approved these changes

@Fuzzbawls Fuzzbawls Fuzzbawls left review comments

Assignees

@random-zebra random-zebra

Labels
Block Generation Mining/Staking related issues Consensus Protocol-Update
Projects
None yet
Milestone
4.0.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@random-zebra @furszy @Warrows @Fuzzbawls @Mrs-X