blockchain: Reverse utxo set semantics. #1471

davecgh · 2018-09-27T08:48:13Z

NOTE: This PR currently requires a full chain download. If you plan to test it, please make a copy of your existing data directory first.

Testing Notes

This PR does not yet have database migration code and is completely incompatible with the current v4 database format. Thus a full chain download is required. dcrd will exit with error in the case you attempt to run it against an existing v4 database and the new database it creates will be v5, so if you try to run an older software version against the new database, you will get an error message similar to Unable to start server on [:9108]: the current blockchain database is no longer compatible with this version of the software (5 > 4).

The migration code will be added via a separate PR.

This is the biggest change to dcrd to date and, as such, will need a significant amount of testing and careful review.

This modifies the way the unspent transaction output set is handled to reverse its current semantics so that it is optimized for the typical case, provides simpler handling, and resolves various issues with the previous approach. In addition, it updates the transaction, address, and existsaddress indexes to no longer remove entries from blocks that have been disapproved as, in all cases, the data still exists in the blockchain and thus should be queryable via the indexes even though there is special handling applied which treats them as if they did not exist in certain regards.

Prior to this change, transactions in the regular tree were not applied to the utxo set until the next block was processed and did not vote against them. However, that approach has several undesirable consequences such as temporarily "invisible" utxos that are actually spendable, disapproved transactions missing from indexes even though they are still in the blockchain, and poor performance characteristics.

In a certain sense, the previous approach could be viewed as the transactions not being valid until they were approved, however, that is not really true because it was (and still is) perfectly acceptable to spend utxos created by transactions in the regular tree of the same block so long as they come before the transactions that spend them. Further, utxos from a transaction in the regular tree of a block can be spent in the next block so long as that block does not disapprove them, which further illustrates that the utxos are actually valid unless they are disapproved.

Consequently, this modifies that behavior to instead make the utxo set always track the most recent block and remove the regular transactions in the parent when a block votes against them. This approach is significantly more efficient for the normal case where the previous block is not disapproved by its successor.

Also, the terminology is changed in several places to refer to disapproved blocks and transaction trees as opposed to invalid, because invalid implies the tree/block is malformed or does not follow the consensus rules. On the contrary, when a block votes against its parent, it is only voting against regular transaction tree of the parent. Both the block and transaction tree are still valid in that case, only the regular transaction tree is treated as if it never existed in terms of effects on the utxo set and duplicate transaction semantics.

High level overview of changes:

Modify the utxo viewpoint to reverse semantics as previously described
- Remove all code related to stake viewpoints
- Change all block connection code in the viewpoint to first undo all
  transactions in the regular tree of the parent block if the current
  one disapproves it then connect all of the stake txns followed by
  the regular transactions in the block
  - NOTE: The order here is important since stake transactions are not
    allowed to spend outputs from the regular transactions in the same
    block as the next block might disapprove them
- Change all block disconnection code in the viewpoint to first undo
  all the transactions in the regular and stake trees of the block
  being disconnected, and then resurrect the regular transactions in
  the parent block if the block being disconnected disapproved of it
- Introduce a new type named viewFilteredSet for handling sets
  filtered by transactions that already exist in a view
- Introduce a function on the viewpoint for specifically fetching the
  inputs to the regular transactions
- Update mempool block connection and disconnection code to match the
  new semantics
- Update all tests to handle the new semantics
Modify the best state number of transactions to include all
transactions in all blocks regardless of disapproval because they
still had to be processed and still exist in the blockchain
Remove include recent block parameter from mempool.FetchTransaction
since the utxoset now always includes the latest block
- This also has the side effect of correcting some unexpected results
  such as coinbases in the most recent block being incorrectly
  reported as having zero confirmations
Modify mempool utxo fetch logic to use a cached disapproved view, when
needed, rather than recreating the view for every new transaction
added to it
Update spend journal to include all transactions in the block instead
of only stake transactions from the current block and regular
transactions from the parent block
Modify tx and address indexes to store the block index of each tx
along with its location within the files and update the query
functions to return the information as well
Change the tx, address, and existsaddress indexes to index all
transactions regardless of their disapproval
- This also corrects several issues such as the inability to query and
  retrieve transactions that exist in a disapproved block
Update all RPC commands that return verbose transaction information
to set that newly available block index information properly
Rename IsRegTxTreeKnownDisapproved in the mining.TxSource interface to
IsRegTxTreeKnownDisapproved
- NOTE: This will require a major bump to the mining module before
  the next release
Rename several utxoView instances to view for consistency
Rename several variables that dealt with disapproved trees from
names that contained Invalid to ones that contain Disapproved

This is work towards #1145.
Fixes #618.

chappjc · 2018-09-27T14:02:44Z

Just completed a full block download successfully, in almost exactly 30 minutes with a single LAN peer. I will be testing a fresh sync of dcrdata with this.

davecgh · 2018-09-27T16:36:25Z

Some additional details on one of the issues this also fixes:

Before:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[0] | dcrctl getrawtransaction - 1
{
  "txid": "0a1a046784c0cbe2e639dc8b79debe2def79b99e2f4ac8a6b055271a77843925",
...
 "vin": [
...
 ],
"vout": [
...
 ],
...
 "blockheight": 0               <----- This is a bug fixed by this PR
...
}

After:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[0] | dcrctl getrawtransaction - 1
{
  "txid": "0a1a046784c0cbe2e639dc8b79debe2def79b99e2f4ac8a6b055271a77843925",
...
  "vin": [
...
  ],
  "vout": [
...
  ],
...
  "blockheight": 278164,
...
}

davecgh · 2018-09-27T16:39:13Z

Some additional details on another one of the issues this also fixes:

Before:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[0] | dcrctl gettxout - 0
{
  "bestblock": "0000000000000000396d8aba7c7c706562c4b962608812d69347af8cd2e05a94",
  "confirmations": 0,                       <----- This is a bug fixed by this PR
...
  "coinbase": true
}

After:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[0] | dcrctl gettxout - 0
{
  "bestblock": "0000000000000000396d8aba7c7c706562c4b962608812d69347af8cd2e05a94",
  "confirmations": 1,
...
  "coinbase": true
}

davecgh · 2018-09-27T19:32:19Z

Some additional details on a third issue this also fixes:

Before:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[1] | dcrctl getrawtransaction - 1 | jq .vin
[
 {
   "txid": "211eec2881df9364ddbaf9cf0fd73cb4a43718ac46fe8fd20c0b3e4ba5d16856",
   ...
   "amountin": -1e-08,                         <--- Not filled in despite being mined
   "blockheight": 0,                           <--- Not filled in despite being mined
   "blockindex": 4294967295,                   <--- Not filled in despite being mined
   ...
 },
 ...
]

After:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[1] | dcrctl getrawtransaction - 1 | jq .vin
[
 {
   "txid": "211eec2881df9364ddbaf9cf0fd73cb4a43718ac46fe8fd20c0b3e4ba5d16856",
   ...
   "amountin": 94.24934123,
   "blockheight": 278198,
   "blockindex": 2,
   ...
 },
 ...
]

matheusd

First pass review, only a few initial comments.

blockchain/indexers/txindex.go

blockchain/utxoviewpoint.go

blockchain/indexers/txindex.go

blockchain/utxoviewpoint.go

blockchain/indexers/existsaddrindex.go

blockchain/indexers/manager.go

blockchain/utxoviewpoint.go

blockchain/indexers/txindex.go

blockchain/utxoviewpoint.go

davecgh · 2018-10-01T16:52:57Z

Some additional details on a fourth issue this also fixes:

Before:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[1] | dcrctl getrawtransaction - 1 | jq -r '.blockhash'
null    <---- The block hash is not filled in despite being mined

After:

$ dcrctl getbestblockhash | dcrctl getblock - | jq -r .tx[1] | dcrctl getrawtransaction - 1 | jq -r '.blockhash'
0000000000000000b76446ea746e2a48de8f1d43cb4674725e37d044865ff800

matheusd

Performing functional testing in simnet now.

Found 2 issues so far

Txs not being re-inserted into mempool after disapproving blocks due to a small bug in blockmanager (noted in the code)
I'm triggering an "assertion failed: missing spend journal data" on reorgs (explanation below)

I'm using this script to test reorgs: https://gist.github.com/matheusd/73907f551b21104bc3dea1af7d8bc385

This is the procedure triggering the issue (btw I'm testing with the first bug corrected as noted in the code review):

Wallets configured to vote false on blocks with blockheight % 10 >= 5 && <= 7
Mine on nodes until after SVH and up to the first block voted false in a streak (eg: 165)
Disconnect nodes
Mine on block beta causing a fork at block 165. Mine on beta until block 167.
Reconnect nodes
Node Alpha receives the new blocks with higher pow, causing a reorg and triggering the error

I've recorded a video describing the error, can send it to you if needed.

blockmanager.go

davecgh · 2018-10-08T06:30:21Z

The issue regarding reorgs of disapproved blocks as reported by @matheusd has been corrected. In addition, tests for the scenario that triggered it have been added via PR #1485 so the scenario is proven to work the same both before and after this PR.

davecgh · 2018-10-08T15:57:53Z

All review items until this point have been addressed.

matheusd

Re-tested after fixes and didn't hit the error anymore. Tested again the utxo status changes and mempool behavior on disapproved blocks and also didn't see any new problems.

Super nice to have these changes in!

davecgh · 2018-10-10T03:29:44Z

Rebased to deal with conflicts with latest master.

davecgh · 2018-10-16T19:00:26Z

Some timing information for the large full block reorg tests with and without these changes:

without changes: 3m21.801s
with changes:    2m18.629s

davecgh · 2018-10-16T19:38:03Z

Rebased to handle merge conflicts with latest master and squashed all approved review updates.

This modifies the way the unspent transaction output set is handled to reverse its current semantics so that it is optimized for the typical case, provides simpler handling, and resolves various issues with the previous approach. In addition, it updates the transaction, address, and existsaddress indexes to no longer remove entries from blocks that have been disapproved as, in all cases, the data still exists in the blockchain and thus should be queryable via the indexes even though there is special handling applied which treats them as if they did not exist in certain regards. Prior to this change, transactions in the regular tree were not applied to the utxo set until the next block was processed and did not vote against them. However, that approach has several undesirable consequences such as temporarily "invisible" utxos that are actually spendable, disapproved transactions missing from indexes even though they are still in the blockchain, and poor performance characteristics. In a certain sense, the previous approach could be viewed as the transactions not being valid until they were approved, however, that is not really true because it was (and still is) perfectly acceptable to spend utxos created by transactions in the regular tree of the same block so long as they come before the transactions that spend them. Further, utxos from a transaction in the regular tree of a block can be spent in the next block so long as that block does not disapprove them, which further illustrates that the utxos are actually valid unless they are disapproved. Consequently, this modifies that behavior to instead make the utxo set always track the most recent block and remove the regular transactions in the parent when a block votes against them. This approach is significantly more efficient for the normal case where the previous block is not disapproved by its successor. Also, the terminology is changed in several places to refer to disapproved blocks and transaction trees as opposed to invalid, because invalid implies the tree/block is malformed or does not follow the consensus rules. On the contrary, when a block votes against its parent, it is only voting against regular transaction tree of the parent. Both the block and transaction tree are still valid in that case, only the regular transaction tree is treated as if it never existed in terms of effects on the utxo set and duplicate transaction semantics. High level overview of changes: - Modify the utxo viewpoint to reverse semantics as previously described - Remove all code related to stake viewpoints - Change all block connection code in the viewpoint to first undo all transactions in the regular tree of the parent block if the current one disapproves it then connect all of the stake txns followed by the regular transactions in the block - NOTE: The order here is important since stake transactions are not allowed to spend outputs from the regular transactions in the same block as the next block might disapprove them - Change all block disconnection code in the viewpoint to first undo all the transactions in the regular and stake trees of the block being disconnected, and then resurrect the regular transactions in the parent block if the block being disconnected disapproved of it - Introduce a new type named viewFilteredSet for handling sets filtered by transactions that already exist in a view - Introduce a function on the viewpoint for specifically fetching the inputs to the regular transactions - Update mempool block connection and disconnection code to match the new semantics - Update all tests to handle the new semantics - Modify the best state number of transactions to include all transactions in all blocks regardless of disapproval because they still had to be processed and still exist in the blockchain - Remove include recent block parameter from mempool.FetchTransaction since the utxoset now always includes the latest block - This also has the side effect of correcting some unexpected results such as coinbases in the most recent block being incorrectly reported as having zero confirmations - Modify mempool utxo fetch logic to use a cached disapproved view, when needed, rather than recreating the view for every new transaction added to it - Update spend journal to include all transactions in the block instead of only stake transactions from the current block and regular transactions from the parent block - Modify tx and address indexes to store the block index of each tx along with its location within the files and update the query functions to return the information as well - Change the tx, address, and existsaddress indexes to index all transactions regardless of their disapproval - This also corrects several issues such as the inability to query and retrieve transactions that exist in a disapproved block - Update all RPC commands that return verbose transaction information to set that newly available block index information properly - Rename IsRegTxTreeKnownDisapproved in the mining.TxSource interface to IsRegTxTreeKnownDisapproved - NOTE: This will require a major bump to the mining module before the next release - Rename several utxoView instances to view for consistency - Rename several variables that dealt with disapproved trees from names that contained Invalid to ones that contain Disapproved NOTE: This does not yet have database migration code and thus will require a full chain download. It will exit with error in the case you attempt to run it against an existing v4 database. The new database it creates will be v5, so attempting to run an older version will reject the new database to prevent corruption. The database migration will be added in a separate commit.

davecgh · 2018-11-09T23:31:53Z

This is ready for final review and approvals.

chappjc · 2018-11-12T17:58:13Z

This branch has been in use on my dev machine and on alpha.dcrdata.org for weeks without issue.

dajohi

ok

davecgh added this to the 1.4.0 milestone Sep 27, 2018

davecgh force-pushed the blockchain_reverse_utxo_semantics branch from 9f5c227 to f28960c Compare September 27, 2018 09:10

matheusd reviewed Sep 28, 2018

View reviewed changes

blockchain/indexers/txindex.go Show resolved Hide resolved

blockchain/utxoviewpoint.go Outdated Show resolved Hide resolved

blockchain/utxoviewpoint.go Outdated Show resolved Hide resolved

blockchain/utxoviewpoint.go Show resolved Hide resolved

dnldd suggested changes Sep 28, 2018

View reviewed changes

blockchain/indexers/txindex.go Outdated Show resolved Hide resolved

blockchain/utxoviewpoint.go Outdated Show resolved Hide resolved

blockchain/utxoviewpoint.go Outdated Show resolved Hide resolved

blockchain/utxoviewpoint.go Outdated Show resolved Hide resolved

davecgh force-pushed the blockchain_reverse_utxo_semantics branch 2 times, most recently from c882fcc to 453f4b4 Compare September 28, 2018 20:01

davecgh commented Sep 28, 2018

View reviewed changes

davecgh force-pushed the blockchain_reverse_utxo_semantics branch from 453f4b4 to c9f67d8 Compare September 28, 2018 20:08

matheusd reviewed Oct 5, 2018

View reviewed changes

blockmanager.go Outdated Show resolved Hide resolved

blockmanager.go Outdated Show resolved Hide resolved

davecgh force-pushed the blockchain_reverse_utxo_semantics branch from 35e02c7 to 70c848b Compare October 8, 2018 06:28

davecgh force-pushed the blockchain_reverse_utxo_semantics branch from 70c848b to d7a4829 Compare October 8, 2018 15:58

matheusd approved these changes Oct 9, 2018

View reviewed changes

davecgh force-pushed the blockchain_reverse_utxo_semantics branch from d7a4829 to db686ba Compare October 10, 2018 03:29

davecgh mentioned this pull request Oct 10, 2018

Recently confirmed transactions do not show recent data decred/dcrdata#142

Closed

chappjc mentioned this pull request Oct 11, 2018

Transactions return 0 confirmations and no timestamp until 2 confirmations decred/dcrdata#401

Closed

davecgh force-pushed the blockchain_reverse_utxo_semantics branch from db686ba to dbea7b2 Compare October 16, 2018 19:37

davecgh mentioned this pull request Oct 16, 2018

blockchain: Convert to direct single-step reorgs. #1500

Merged

davecgh force-pushed the blockchain_reverse_utxo_semantics branch from dbea7b2 to df1898c Compare November 9, 2018 23:27

davecgh changed the title ~~WIP: blockchain: Reverse utxo set semantics.~~ blockchain: Reverse utxo set semantics. Nov 9, 2018

This was referenced Nov 9, 2018

multi: Migration for utxo set semantics reversal. #1520

Merged

Multi-peer Checklist #1145

Open

dnldd approved these changes Nov 10, 2018

View reviewed changes

alexlyp approved these changes Nov 12, 2018

View reviewed changes

dajohi approved these changes Nov 12, 2018

View reviewed changes

davecgh merged commit df1898c into decred:master Nov 12, 2018

davecgh deleted the blockchain_reverse_utxo_semantics branch November 12, 2018 22:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blockchain: Reverse utxo set semantics. #1471

blockchain: Reverse utxo set semantics. #1471

davecgh commented Sep 27, 2018 •

edited

Loading

chappjc commented Sep 27, 2018

davecgh commented Sep 27, 2018

davecgh commented Sep 27, 2018

davecgh commented Sep 27, 2018

matheusd left a comment

davecgh commented Oct 1, 2018

matheusd left a comment •

edited

Loading

davecgh commented Oct 8, 2018

davecgh commented Oct 8, 2018

matheusd left a comment

davecgh commented Oct 10, 2018

davecgh commented Oct 16, 2018

davecgh commented Oct 16, 2018

davecgh commented Nov 9, 2018

chappjc commented Nov 12, 2018

dajohi left a comment

blockchain: Reverse utxo set semantics. #1471

blockchain: Reverse utxo set semantics. #1471

Conversation

davecgh commented Sep 27, 2018 • edited Loading

NOTE: This PR currently requires a full chain download. If you plan to test it, please make a copy of your existing data directory first.

Testing Notes

chappjc commented Sep 27, 2018

davecgh commented Sep 27, 2018

davecgh commented Sep 27, 2018

davecgh commented Sep 27, 2018

matheusd left a comment

Choose a reason for hiding this comment

davecgh commented Oct 1, 2018

matheusd left a comment • edited Loading

Choose a reason for hiding this comment

davecgh commented Oct 8, 2018

davecgh commented Oct 8, 2018

matheusd left a comment

Choose a reason for hiding this comment

davecgh commented Oct 10, 2018

davecgh commented Oct 16, 2018

davecgh commented Oct 16, 2018

davecgh commented Nov 9, 2018

chappjc commented Nov 12, 2018

dajohi left a comment

Choose a reason for hiding this comment

davecgh commented Sep 27, 2018 •

edited

Loading

matheusd left a comment •

edited

Loading