Started the long road here, have reviewed 1/4 of it. Good stuff 🥃 .

Found few problems (aside from the comments) that have gone deeper on a personal branch repo link.

Essentially, was validating that votes for regular masternodes and deterministic masternodes were expiring properly (added functional test coverage for it in ef1f9afbc942bcdb58f69175435c7900ae6eace3) and found out that the COutPoint returned by the DMN setup in the test framework suite had the index hardcoded to 0, which was making the test fail sometimes as the collateral output index isn't fixed to any position in the tx creation process (fixes bfa6fa2b18d94e1b0a063497eaf601bc4f8dcd70 and ace871f276e2c193c8714f2665c073effb75f09d).
-- All yours to cherry-pick the three commits that did into this branch ☕ ☕ --

Aside from that, noticed that for some reason that haven't found yet, DMNs aren't removed from the list right away after a collateral spending. It takes a bit of time. Idk if you already have done it in a future PR but will be good to create a test similar to the tiertwo_masternode_activation.py but for DMN and verify every possible status and activation/deactivation/removal possible errors (ofc that this work is for another PR).

Thanks a lot for the review @furszy. 🙏
I know that this one is probably the hardest PR of the list, and we should take our time, as it's easy to introduce bugs here.
So kudos for the test-driven approach 🥃

I think that what you suggest, though, goes in the wrong direction, due to a confusion between ProTx and collateral.

found out that the COutPoint returned by the DMN setup in the test framework suite had the index hardcoded to 0

Yes. It's on purpose.
That is not the collateral outpoint. It is just the hash of the proRegTx (collateral and proTx are different things, their txid is the same only in the "fund" case, not in "internal" or "external" case). We are returning a COutPoint in setupDMN just for compatibility with the old system (setupMasternode), but we only ever use its hash in the test (self.proRegTx1.hash, self.proRegTx2.hash, self.proRegTx3.hash).

I realize now that this is probably confusing, so have reworked setupDMN to return only the (proTx) hash -as it actually was already doing before 64d09e55aaa621045e93b3d26405cbcd632111b7-. Now it should be clearer.

This setup function is used exclusively in the compatibility test (and, as the comment states, it will be removed with it).
In the new tiertwo_deterministicmns.py (#2345) we use a different initialization, returning objects of the Masternode class introduced at the end of this PR, which encapsulates (among other things) the collateral out.

Aside from this, inserting an "index" in all the ProTx RPC outputs, as you did, is conceptually wrong imo, and it would be confusing for the user, because, in all cases, except for protx_register_fund:

• the index refers to another transaction, not the one identified by the returned "txid"
• the user already knows the collateral vout, as it is one of the RPC input parameters.

The reason why your test was failing is because this is wrong:

self.log.info("expiring DMN..")
self.spend_collateral(self.ownerOne, self.proRegTx1, self.miner)
self.wait_until_mn_vinspent(self.proRegTx1.hash, 30, [self.remoteTwo])

You shouldn't pass self.proRegTx as collateral outpoint to spend (even though the first DMN incidentally was registered with "fund").
As explained, the output returned by setupDMN is not the collateral.
In the deterministic tests the collateral outpoint can be accessed from the Masternode object (dmn.collateral). Here we can just retrieve it with the RPC before spending.

Another side-note for the test is with this:

self.stake(12, [self.remoteTwo]) # create blocks to remove staled votes

NewBlock is called every 14 blocks, not 12.
Technically we could even not stake, after spending the collateral, and simply force RemoveStaleVotesOnProposal by calling GetBudget() via RPC, but still... better to cover NewBlock.

Have picked and fixed the first commit from your branch.
The test is passing fine now.

Idk if you already have done it in a future PR

#2345 introduces lots more testing (e.g. check b28fc43445b0556af2f9b079f9a670b160a30bf3 for collateral spending)

After this first milestone, I think we can start working on a separate functional test for the governance, based only on DMNs.
This way, when the legacy system is obsolete, we can just remove tiertwo_governance_sync_basic.py (as well as tiertwo_mn_compatibility.py), and keep only the new tests.

still digging through this one, quite a lot to digest, but wanted to leave some quick comments

A bit more feedback.

Another day here, getting closer to finish it ☕ ☕ . Left some more feedback.

ACK 04cd17e ☕.

Future work: add test cases for the invalid mnw and mnv broadcast paths.

ACK 04cd17e