Segwit light #61
Segwit light #61
Conversation
|
Even after activating the fork, almost all regtests still pass. Only the Before merging, that last commit should be removed from the PR and just applied back after fork activation (so the tests then still pass). |
domob1812
force-pushed
the
segwit-light
branch
3 times, most recently
from
6bc57a7 to
2881b44
Compare
|
This is now ready for review. |
divi/src/main.cpp
Outdated
| inputs.ModifyCoins(tx.GetHash())->FromTx(tx, nHeight); | ||
| const uint256 outputHash = (as.IsActive(Fork::SegwitLight) | ||
| ? tx.GetBareTxid() : tx.GetHash()); | ||
| inputs.ModifyCoins(outputHash)->FromTx(tx, nHeight); |
There was a problem hiding this comment.
This doesn't seem very safe from an upgradability perspective. It seems like it is possible that an older transaction that was formerly referred to by legacy hash will now fail to be found by updated hash
There was a problem hiding this comment.
I'm not sure I understand your concern here. A transaction will always be referred to based on the activation state at the block it was confirmed in. There's no ambiguity there, and also e.g. the wallet will be able to properly handle spending any coins, created before or after the fork.
Perhaps a "weird" thing that could happen is if there is a reorg right around the fork, then a transaction might have been confirmed with e.g. old rules on one fork, and with new rules on another. But even if that happens (which seems really unlikely in practice), that wouldn't cause any issues anywhere.
divi/src/txmempool.cpp
Outdated
| std::map<COutPoint, CInPoint>::iterator it = mapNextTx.find(COutPoint(origTx.GetHash(), i)); | ||
| auto it = mapNextTx.find(COutPoint(segwitLight ? origTx.GetBareTxid() : origTx.GetHash(), i)); | ||
| if (it == mapNextTx.end()) |
There was a problem hiding this comment.
Mempool logic is a bit broken around the fork, indeed. But with the restriction we have put in place, it will be fine in practice.
For this particular place here (and the next one below), we could of course just try to look up both hashes and use the one that works (if any). Is that something you would prefer?
divi/src/txmempool.cpp
Outdated
| if (lookup(txin.prevout.hash, tx2)) { | ||
| if ((segwitLight && lookupBareTxid(txin.prevout.hash, tx2)) | ||
| || (!segwitLight && lookup(txin.prevout.hash, tx2))) { |
There was a problem hiding this comment.
In the event of timestamps not being monotonic, a transaction that is saved in one block under 'segwit-light' will be 'not found' if the next block is not 'segwit-light'
There was a problem hiding this comment.
I can't find this place in the code anymore, but I guess what you mean here is that the mempool logic can be inconsistent around fork activation. This is the case independent of timestamps being monotonic or not, and is why we have this special rule of forbidding spending of unconfirmed outputs in the mempool policy (and wallet) for a time window around the fork. This time window should be large enough to make sure there are no issues like this in practice.
On the core consensus level, non-monotonic timestamps are of course fine as well. For a transaction confirmed in a particular block, that block's timestamp (independent of what came before and comes after) determines what UTXOs get created, and the wallet later also handles this correctly when spending those UTXOs again.
| * but is forced to off around the segwit-light fork. */ | ||
| bool AllowSpendingZeroConfirmationChange() const; |
There was a problem hiding this comment.
Wouldn't it be simpler to do the upgrade process in two separate upgrades?
(1) You change the GetHash function to return the legacy hash if the transaction version is some specific value (the present default) and to return the bare txid for the upgraded version number.
(2) You upgrade the network as a soft fork by not including the updated version number directly.
(3) Once everybody has made the first update, the second update is a softfork relative to the first one.
What are the limitations to doing this?
There was a problem hiding this comment.
That is an interesting suggestion. I don't see any reason why it wouldn't work. However, I believe it would basically just make things more complex without much benefit:
- it would still be a hard fork (one of them, namely step 1), so no benefit in this respect
- it would be two forks instead of one that need to be scheduled, people notified, and hoping that they update in time, which I think rather increases than decreases the overall risk
- instead of a rather simple rule change "in one place" (concerning consensus at least), namely what UTXOs get created for a particular transaction, we now have a more complex dependency between the transaction version, things the version implies, and consensus rules about what versions are allowed
I can see one benefit of this change, and that is fixing the spending of unconfirmed outputs in a time-window "around" fork activation (which is probably why you brought up this suggestion here). However, I would much rather have some temporary logic to avoid issues as in this PR (which, notably, also does not affect the core consensus at all) than more complicated fork logic in general that we need to keep for the future.
There was a problem hiding this comment.
Some clarification on item (1), you wouldn't update the version number in step (1) so that it remains a soft-fork. You'd update it at a later stage after you've ensured enough people have upgraded. Blocks with the hard fork active would only accept newer-version transactions perhaps? Sounds messier I agree. I'm not entirely certain about the best way to go about it.
There was a problem hiding this comment.
I've thought about this a bit more. In fact, I think it needs three forks, which at least conceptually are separate (but of course they could under some circumstances be part of the same code update, just scheduled at different times):
- A soft fork that disallows a certain transaction version completely (or all versions larger than current default).
- A hard fork that changes what UTXOs get generated for transactions with a new version (which aren't valid on the network at this stage at all, so this change is rather easy to do after the network has updated and activated step 1 successfully).
- Another hard fork that actually allows transactions of the new version again.
Step 1 is relatively uncritical as a straight-forward soft fork. But once step 1 is activated on the network, steps 2 & 3 could be done in a follow-up with perhaps a bit less risk and a combined update. They remain a hard fork, though.
I think this process could work, and might be able to reduce the upgrade risk. But I'm still not fully convinced it is the best way forward; the only real benefit would be removing that temporary restriction of spending unconfirmed outputs around the activation of the current PR. And the version-number-dependent logic in itself is then something a bit more complex that remains forever.
domob1812
force-pushed
the
segwit-light
branch
5 times, most recently
from
93b7179 to
6cc0d44
Compare
|
I've started to work on splitting out some more parts of this PR. Marking as draft in the mean time (but the discussions started here can of course continue). |
domob1812
force-pushed
the
segwit-light
branch
14 times, most recently
from
5760e8e to
81fb3d8
Compare
domob1812
force-pushed
the
segwit-light
branch
4 times, most recently
from
8874920 to
366efa7
Compare
domob1812
force-pushed
the
segwit-light
branch
3 times, most recently
from
f86d40f to
c3e535a
Compare
Wrap some of the functions used only in main into an anonymous namespace; some of them were already marked as "static", others were not (but they were still not needed from anywhere else). This makes it immediately clear that those functions are only used in the context of main. FindUndoPos had a declaration first and the definition later, but it is fine to just move the definition to where the declaration was. This simplifies the code further (and is a pure move in the context of this commit).
This is a bunch of related and straight-forward cleanups to the code around masternode configuration: Pass arguments as const& instead of by value, mark functions as const, and move helper functions from being static in a class and declared inside the header to just being inside an anonymous namespace in the cpp file.
Some places in the code need to determine the hash by which the UTXOs of a given transaction will be referred to; in particular, we need that when processing UTXO set updates for block connects and disconnects, in the mempool and for assembling transactions into a new block. This commit introduces a class TransactionUtxoHasher, which abstracts this step and is used in all those places in the code instead of just getting the txid directly. For now, this has no effects on behaviour; but it makes it more clear in the code where we need this particular logical feature; it will allow us to add some more unit tests for those parts with explicit mocks of the hasher class; and it will make it easier to implement segwit-light in the future (where we basically just need to flip the hasher implementation but no other parts in the code).
Add a unit test (together with the necessary framework around) for UTXO creation from UpdateCoins, based on the UTXO hasher (rather than the txid directly).
This extends the mempool unit tests to explicitly verify that adding transactions, removing transactions, checking the pool and looking up coins / transactions still works even if we use the bare txid for some transactions as UTXO hash (as will be the case with segwit-light in the future).
Use the UTXO hasher abstraction in the wallet and for staking (i.e. for places where coins are spent). The wallet gets its own instance, which will allow for dependency injection in tests. For now, the hasher used in the wallet is just the normal hasher, i.e. there are no actual changes in behaviour. In the future, the wallet hasher can be changed accordingly for the activation of segwit light.
This adds basic unit tests for PoSTransactionCreator, with a fake environment that allows a real PoSTransactionCreator to create staking transactions. In particular, we use that to unit test that it uses the UTXO hasher of the wallet properly.
Extend the unit tests in wallet_coinmanagement_tests.cpp to include also explicit checks for situations in which the wallet is supposed to the UTXO hasher rather than e.g. a plain transaction hash.
This updates the regtests to use "outputhash" for listunspent results in some places, to make sure the code will also work after activating segwit-light. Some other places remain where outputs are not from listunspent and that still needs to be updated when segwit-light gets activated generally, but this is a first step to reduce the amount of required changes then.
This implements "segwit-light" in the node (consensus and mempool): After activation of a new fork, outputs created by new transactions are referred to by the creating transaction's bare txid rather than the normal hash (txid). This makes chains of constructed transactions immune to transaction malleability. In the mempool, we use the current chain tip to determine the activation state of the fork, as we can't know for sure when a transaction will be confirmed (and thus what activation state will be in effect then). This will lead to issues right around the fork activation time, but we will simply disallow spending of unconfirmed outputs in the mempool and wallet "around" the fork time temporarily to resolve this. The wallet is also not yet updated to take the change into account when selecting coins.
Around the segwit-light fork (some hours before and after), we should avoid spending unconfirmed outputs: If we do that, it is unclear whether or not the fork will be active in the block that confirms those spends, and thus we can't reliably construct a valid chain. With this change, we introduce a (temporary) measure to disallow those spends in the wallet and mempool policy in a window of time around the planned fork activation.
Handle segwit-light properly when spending coins, either for staking or normal transactions in the wallet. Depending on when a transaction was confirmed, we need to make sure to include the right outpoint (with txid or bare txid) in the transaction spending an output. This is done through a custom UTXO hasher used in the wallet, which specifically works for CWalletTx.
This extends the two functions GetWalletTx and GetTransaction to (also) find transactions by bare txid, not just by the normal txid. These methods are mainly used in places where we need to look up e.g. the previous transaction to a spend, so that we can know the address that is being spent or the value of the input. The change is fine to do (won't cause any extra consensus changes) because all it does is make those methods return the correct previous transaction (for after the fork) in cases where they would have failed otherwise (since both are SHA-256d hashes and thus cannot have collisions). Also actual checks that some spent coin actually exists are the explicitly on the consensus-level anyway.
When the transaction index is enabled (-txindex), this change also keeps an index from bare txid to the disk position, so that lookups with the index work on both normal txids and bare txids. With this, the previous change made to GetTransaction will be improved for enabled -txindex, and it will also allow us to look up any transaction by bare txid, even if the fallbacks in GetTransaction fail.
This extends the txindex.py test, to check lookups also by bare txid where this is possible (mempool and txindex). Lookups by UTXO set only work on whatever hash is used for the UTXO before/after segwit light.
This adds the new regtest segwit_light.py, which verifies that sending of transactions and staking still works after the fork, and that the new rules work as expected. It also explicitly checks that transaction malleability is not an issue anymore for chains of pre-signed transactions.
|
Reopened in #82 with corrected updated branches. |
This implements "segwit light" as described in the design doc. We create a new time-based fork (for now scheduled at block time 2'000'000'000, to be changed to a real timestamp later); all transactions made after the fork create UTXOs not indexed by their normal hash (txid), but instead by a hash not including any signature data (bare txid). This means that all transactions made after the fork can be safely used in a chain of unconfirmed, pre-signed transactions and won't be affected by malleability.