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

V0.12.0.x use specific proto and fix sync issues #441

Merged
merged 6 commits into from
Jul 20, 2015
Merged

V0.12.0.x use specific proto and fix sync issues #441

merged 6 commits into from
Jul 20, 2015

Conversation

UdjinM6
Copy link

@UdjinM6 UdjinM6 commented Jul 18, 2015

  • store all MNs that fit min payment proto, use only those who fit specific proto (pool, ix, budget)
  • use pindexBestHeader height to identify recent block instead of magic time <--- doesn't play nice with forks
  • reset last... on sync initial state
  • update lastMasternodeList when we received legit update for MN we already know
  • ask for mnb on mnp even if not synced yet
  • change rpc and "tools -> info" to show "ds / enabled / all" for count

@UdjinM6
Copy link
Author

UdjinM6 commented Jul 19, 2015

Don't merge it yet, having issues syncing during forks

//don't begin syncing until we're at a recent block
if(pindexPrev->nTime + 600 < GetTime()) return;
//don't begin syncing until we're almost at a recent block
if(pindexPrev->nHeight + 4 < pindexBestHeader->nHeight || pindexPrev->nTime + 600 < GetTime()) return;

Choose a reason for hiding this comment

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

Shouldn't we wait to ask for mnb/mbp until we have the most recent block? Otherwise if a masternodes input is newer than the current block, we will reject it and have inconsistancies

Copy link
Author

Choose a reason for hiding this comment

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

Well, I agree. I'm not sure on that either tbh. I just divided 600 by blocktime (i.e. 150) and that's where 4 came from :)
On another note we are just entering cycle to ask other nodes for MNs so most likely we'll be already at a recent block when the first mnb from them arrive.

Choose a reason for hiding this comment

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

Gotcha, shouldn't it be "pindexPrev->nHeight + 4 > pindexBestHeader->nHeight" though? Being that we get the headers first and we're checking to see when the chain actually downloads, right?

Copy link
Author

Choose a reason for hiding this comment

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

err... nope. Say we have 100 headers and only 10 blocks so
if (10 + 4 < 100 || ...) return; // do not sync yet
Or am I missing smth?
I would agree however that it's better to use && here
if(pindexPrev->nHeight + 4 < pindexBestHeader->nHeight && pindexPrev->nTime + 600 < GetTime()) return;
i.e. if block height is still low AND block time is low too THEN do nothing. Thoughts?

Choose a reason for hiding this comment

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

Oh, I just was reading it backwards somehow :) I think using && is probably
a lot better in that case

On Sun, Jul 19, 2015 at 7:46 AM, UdjinM6 notifications@github.com wrote:

In src/masternode-sync.cpp
#441 (comment):

  •        //don't begin syncing until we're at a recent block
    
  •        if(pindexPrev->nTime + 600 < GetTime()) return;
    
  •    //don't begin syncing until we're almost at a recent block
    
  •    if(pindexPrev->nHeight + 4 < pindexBestHeader->nHeight || pindexPrev->nTime + 600 < GetTime()) return;
    

err... nope. Say we have 100 headers and only 10 blocks so
if (10 + 4 < 100 || ...) return; // do not sync yet
Or am I missing smth?
I would agree however that it's better to use && here
if(pindexPrev->nHeight + 4 < pindexBestHeader->nHeight &&
pindexPrev->nTime + 600 < GetTime()) return;
i.e. if block height is still low AND block time is low too THEN do
nothing. Thoughts?


Reply to this email directly or view it on GitHub
https://github.com/dashpay/dash/pull/441/files#r34958712.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, np, happens to me too sometimes 😄
changed to &&, see next commit

@UdjinM6
Copy link
Author

UdjinM6 commented Jul 19, 2015

rebased

@UdjinM6
Copy link
Author

UdjinM6 commented Jul 20, 2015

Ok, rebased again and also removed -masternodeminprotocol and nMasternodeMinProtocol
It's better to use masternodePayments.GetMinMasternodePaymentsProto() (using it in CountEnabled() by default now) to ensure everyone is on the same page.

eduffield222 pushed a commit that referenced this pull request Jul 20, 2015
V0.12.0.x use specific proto and fix sync issues
@eduffield222 eduffield222 merged commit a831418 into dashpay:v0.12.0.x Jul 20, 2015
@UdjinM6 UdjinM6 deleted the v0.12.0.x_proto_and_sync branch March 7, 2016 03:12
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Mar 23, 2022
…igner

2efdfb8 gui: restore Send for external signer (Sjors Provoost)
4b5a6cd refactor: helper function signWithExternalSigner() (Sjors Provoost)
026b5b4 move-only: helper function to present PSBT (Sjors Provoost)

Pull request description:

  Fixes dashpay#551

  For the simplest use case of a wallet with one external signer and "PSBT Controls" disabled in settings (the default), the send dialog will behave the same as when using a wallet with private keys. I.e. there's no "Create Unsigned" button.

  When PSBT controls are turned on, you can now actually make a PSBT with signing it; before this PR that button would trigger a sign event and would broadcast the transaction.

  In case a multisig, the Send button will sign on the device, and then fall back to presenting a PSBT (same behavior as before dashpay#441).

  This PR starts with two refactoring commits to move some stuff into a helper function for improved readability in general, and to make the main commit easier to review.

ACKs for top commit:
  jonatack:
    utACK 2efdfb8 diff review since my last review, code re-review, rebased to current master, verified clean debug build of each commit
  luke-jr:
    utACK 2efdfb8

Tree-SHA512: e8731a0ef9e87564b2676c7b022b742d9621bba964c19dba9fd9f6961eb608737a9e1a22c0a3c8b2f2f6d583bba067606ee8392422e82082deefb20ea7b88c7c
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.

3 participants