Nakamoto Tenure Inventories

[jcnelson]

This PR adds the ability for the networking stack to query and generate Nakamoto inventory messages for Nakamoto tenures. It adds GetNakamotoInv to request an inventory for a reward cycle (identified by its start-block consensus hash), and NakamotoInv to reply the bit vector of available inventories. The ith bit is set if the node has the ith processed tenure in the identified reward cycle.

This PR appears big only because it splits up stackslib/src/net/inv.rs into separate epoch 2.x and nakamoto files, and further splits out the unit tests in inv.rs into a separate test file. The following files are unchanged from stackslib/src/net/inv.rs:

• stackslib/src/net/inv/epoch2x.rs -- the epoch 2.x inventory state machine (unchanged)
• stackslib/src/net/inv/tests/epoch2x.rs -- the epoch 2.x inventory state machine tests (unchanged, except to bind on any available ports instead of hard-coded ones).

[codecov]

Codecov Report

Attention: 1135 lines in your changes are missing coverage. Please review.

Comparison is base (074fd76) 83.12% compared to head (22a4567) 70.51%.

Files	Patch %	Lines
stackslib/src/net/tests/inv/epoch2x.rs	40.60%	964 Missing ⚠️
stackslib/src/net/codec.rs	56.84%	63 Missing ⚠️
stackslib/src/net/tests/inv/nakamoto.rs	87.47%	54 Missing ⚠️
stackslib/src/net/chat.rs	93.03%	25 Missing ⚠️
stackslib/src/net/tests/mod.rs	97.61%	14 Missing ⚠️
stackslib/src/chainstate/nakamoto/tests/node.rs	83.33%	8 Missing ⚠️
...kslib/src/chainstate/nakamoto/coordinator/tests.rs	84.21%	3 Missing ⚠️
stackslib/src/chainstate/stacks/db/transactions.rs	33.33%	2 Missing ⚠️
stackslib/src/net/inv/nakamoto.rs	98.18%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             next    #4232       +/-   ##
===========================================
- Coverage   83.12%   70.51%   -12.62%     
===========================================
  Files         438      442        +4     
  Lines      313332   314970     +1638     
===========================================
- Hits       260463   222089    -38374     
- Misses      52869    92881    +40012     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kantai]

This looks mostly good to me -- I had a couple questions and suggestions. There's only one point that I think may require more investigation (the iteration on generating the bit vector) and testing if so.

[kantai]

is cur_tenure_info.parent_tenure_id_consensus_hash always equal to parent_sortition_consensus_hash? I don't think it is... for example, if the prior bitcoin block didn't produce a sortition winner.

In which case, I think this loop's variable maintenance would be broken: cur_height would decrement by 1, cur_consensus_hash would point at the parent bitcoin block, and cur_tenure_opt would be a tenure starting at a different bitcoin block.

My suggestion would be to move cur_tenure_opt's binding into the loop's scope, and then the start of the loop initializes cur_tenure_opt:

loop {
    ...
    if cur_reward_cycle < reward_cycle {
        break;
    }
    let cur_tenure_opt = self.get_processed_tenure(chainstate, &cur_consensus_hash);

I think this makes the loop easier to understand, and eliminates code. Because the tenure_status could just be:

// we've processed the tenure if we have information for it, and the tenure began in this sortition.
let tenure_status = matches!(
    cur_tenure_opt.as_ref(), 
    Some(cur_tenure_info) if cur_tenure_info.tenure_id_consensus_hash == cur_consensus_hash
);
tenure_statuses.push(tenure_status);

Or something along those lines if you don't prefer to use matches!

[jcnelson]

is cur_tenure_info.parent_tenure_id_consensus_hash always equal to parent_sortition_consensus_hash? I don't think it is... for example, if the prior bitcoin block didn't produce a sortition winner.

Yes, correct. That's accounted for in the else-branch of this if/else block. This is also tested via the test_nakamoto_inv_2_tenures_3_sortitions unit tests. You can see that we get a false for one of the sortitions, since it didn't have a corresponding tenure.

[kantai]

The problem is this part of the code:

            if let Some(cur_tenure_info) = cur_tenure_opt.as_ref() {
                // a tenure was active when this sortition happened...
                if cur_tenure_info.tenure_id_consensus_hash == cur_consensus_hash {
                    // ...and this tenure started in this sortition
                    tenure_status.push(true);
                    cur_tenure_opt = self.get_processed_tenure(
                        chainstate,
                        &cur_tenure_info.parent_tenure_id_consensus_hash,
                    )?;
  ...

Combined with this part of the code:

            cur_consensus_hash = parent_sortition_consensus_hash;
            if cur_height == 0 {
                break;
            }
            cur_height = cur_height.saturating_sub(1);

So, for example, if the bitcoin blocks 1 & 3 contained successful sortitions but 2 didn't, after evaluating block 3, the loop would set:

cur_consensus_hash = bitcoin block 2 (because parent_consensus_hash of 3 is 2).
cur_tenure_opt = Some(tenure of bitcoin block 1) (because the parent_tenure_id_consensus_hash of 3 would be 1)
cur_height = 2

So, the three loop variables would be out of sync. Maybe this doesn't create an issue in tests because cur_tenure_info.consensus_hash != consensus_hash, so it pushes tenure_status = false anyways, but it should be observable at some point, because the loop variables do appear to get unsynced.

[jcnelson]

So, the three loop variables would be out of sync.

No, that's deliberate. cur_height tracks the position in the inventory vector, which gets set to true or false based on whether or not cur_tenure_opt matches the consensus hash of the sortition corresponding to cur_height.

Basically, this loop checks each sortition at every height in the reward cycle, and determines from there whether or not the sortition was the start of a tenure. If so, then true gets pushed to the inventory vector. Otherwise, false will be pushed.

I've significantly increased test coverage to synthesize multi-reward-cycle chain histories from inventory vectors, and then verified that the inventory generation logic reproduces the inventory vectors from the synthesized chain history.

[jcnelson]

@kantai @jferrant @xoloki ping 🙏

[kantai]

This LGTM, just add a comment to the loop at inv/nakamoto.rs:158 describing the invariants of the loop variables (cur_tenure_opt, cur_height, and cur_consensus_hash, where cur_tenure_opt if set refers to the active tenure, even if that tenure started before the current height.)

[jferrant]

LGTM

[blockstack-devops]

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.