fix: let internal govobj/vote inv request trackers expire after 60 sec

UdjinM6 commented 2 months ago

Issue being fixed or feature implemented

Another issue noticed during recent sb voting period. Lots of inv are coming from peers that never send the object/vote they announced. Let's not wait for these forever, 60 seconds should be enough.

What was done?

Add a "timer" to each request.

How Has This Been Tested?

Run tests, run a MN on mainnet and check logs.

Breaking Changes

n/a

Checklist:

[x] I have performed a self-review of my own code
[ ] I have commented my code, particularly in hard-to-understand areas
[ ] I have added or updated relevant unit/integration/functional/e2e tests
[ ] I have made corresponding changes to the documentation
[ ] I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

coolaj86 commented 2 months ago

Could someone please explain simply what this fix means in terms of the user pipeline for gobject prepare; gobject submit? \ (which I hope is no change)

Real-World, Implemented-in-Production Use Case

I created the following proposal with a time backdated to match start_epoch (or otherwise time is one day in the past on the same hour as start_epoch):

https://rpc.digitalcash.dev/#?method=gobject&params=["get","831672df4230c3578c10ab4c939d912596ca8b9328a93912ddf596699d6a3af9"]&submit

This was to avoid the following, documented-as-dangerous UX bug:

https://github.com/dashpay/dash/blob/v21.1.x/src/rpc/governance.cpp#L168-L171

    // This command is dangerous because it consumes 5 DASH irreversibly.
    // If params are lost, it's very hard to bruteforce them and yet
    // users ignore all instructions on dashcentral etc. and do not save them...
    // Let's log them here and hope users do not mess with debug.log

The code for generating the timestamps to circumvent that bug is here:

https://github.com/dashhive/DashGov.js/blob/35fc77ecef083776e81367330c57a98760f1f6cd/dashgov.js#L395-L420

/**
   * Creates a draft object with reasonable and default values
   * @param {Uint53} now - use Date.now(), except in testing
   * @param {Uint53} startEpochMs - used to create a deterministic gobject time
   * @param {GObjectData} data - will be sorted and hex-ified
   * @param {GObject} [gobj] - override values
   */
  DashGov.proposal.draft = function (now, startEpochMs, data, gobj) {
    let dataHex = gobj?.dataHex || DashGov.proposal.sortAndEncodeJson(data);
    let time = DashGov.proposal._selectKnownTime(now, startEpochMs);

    /** @type {DashGov.GObject} */
    let normalGObj = {
      hashParent: 0,
      revision: 1,
      time: time,
      dataHex: "",
      masternodeOutpoint: null,
      collateralTxId: null,
      collateralTxOutputIndex: null,
      signature: null,
    };
    Object.assign(normalGObj, gobj, { dataHex });

    return normalGObj;
  };

/**
   * The arbitrary use of random times is a leading cause of lost money during
   * the proposal process, so instead we use the 'start epoch' when appropriate,
   * or otherwise a UTC day interval of the start epoch
   * @param {Uint53} now
   * @param {Uint53} startMs
   */
  DashGov.proposal._selectKnownTime = function (now, startMs) {
    let startEpochDate = new Date(startMs);
    let today = new Date();
    if (today < startEpochDate) {
      let date = today.getUTCDate();
      startEpochDate.setUTCFullYear(today.getUTCFullYear());
      startEpochDate.setUTCMonth(today.getUTCMonth());
      startEpochDate.setUTCDate(date - 1);
    }
    let knownTimeMs = startEpochDate.valueOf();
    let knownSecs = knownTimeMs / 1000;
    knownSecs = Math.floor(knownSecs);

    return knownSecs;
  };

Assumption

I believe what this issue is saying is just that it's going to ignore inventory spam based on actual MN message time from the (supposed) gobject submit that should be sending inventory via p2p and gobject via the MN network simultaneously. So I believe it won't affect this sort of fix, but I'd like an ACK... or a change to the code to not cement the UX bug further in place at the protocol layer.

edit: hmm... not sure why github isn't unfurling those code blocks like it normally does, updated with code

UdjinM6 commented 2 months ago

@coolaj86 this PR has nothing to do with the proposal data itself, it's dealing with an observed misbehaviour on p2p level only. Basically, a peer says "I have a governance object/vote with hash X", we remember this and request it. The object with hash X never arrives but we kept waiting for it forever (not really waiting in networking terms, just remembering that we asked for it earlier). This PR simply lets us forget about such requests after 60 seconds.

PastaPastaPasta commented 1 month ago

let's rebase I guess to have CI green? may just be a flaky failure, restarting.

dashpay / dash