-
Notifications
You must be signed in to change notification settings - Fork 716
PoX: Threshold Adjustment and Reward Address Repetition #1954
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
Conversation
…pox contract at boot. parameterize syncctl wait times for testing
…pox contract at boot. parameterize syncctl wait times for testing
…ockchain into feat/pox-thresholds
5530626 to
f599d0d
Compare
| #[cfg(not(test))] | ||
| const POX_SYNC_WAIT_MS: u64 = 1000; | ||
| #[cfg(test)] | ||
| const POX_SYNC_WAIT_MS: u64 = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this value is anything other than 1000, then the block download and processing time estimations won't work correctly -- the number of samples taken is supposed to be exactly equal to the number of seconds over which the samples were taken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left at 1000ms, the integration tests take about 2x as long to run -- is there a better way to disable the stalling behavior during tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can set a smaller value for ConnectionOptions::timeout. This is what gets used to determine how many samples the PoX sync watchdog takes. The units are in seconds -- you can set this to something like 3 instead of 30 for a local test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made this configurable via 33ec31b
| (define-public (set-burnchain-parameters (first-burn-height uint) (prepare-cycle-length uint) (reward-cycle-length uint) (rejection-fraction uint)) | ||
| (begin | ||
| (asserts! (and is-in-regtest (not (var-get configured))) (err ERR_NOT_ALLOWED)) | ||
| (asserts! (not (var-get configured)) (err ERR_NOT_ALLOWED)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we're getting rid of is-in-regtest, should we kill it completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it for the time being -- it may be useful elsewhere.
lgalabru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thank you @kantai!
jcnelson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This LGTM. Thanks @kantai!
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This PR implements a few different things:
set-burnchain-parameterson the PoX contract.stacks-node, this parameterizes the wait time in the syncctl module.The implementations of 1 and 2 are tested via unit tests, as well as some integration paths in the
poxtest inneon_integrations(which also tests 3).