Fix #377; db must contain hash + type, not just hash.#379
Fix #377; db must contain hash + type, not just hash.#379kleetus merged 1 commit intobitpay:masterfrom
Conversation
|
Note that this changes some internal APIs.
Additionally, |
f4898f4 to
470727c
Compare
|
Here's an easy (low block height) address that shows an erroneous balance: https://test-insight.bitpay.com/address/2N1d4nqHgVUqY5HaUKmRpD69RMeigkYZKk4 |
6d8fc91 to
8f651f0
Compare
|
Rebased due to a test error in _getOutputsMempool, all tests green now. |
|
Good find. And yes for optimization the database stores the hash only, as the probability of a collision between the hash of a public key to the hash of a script is extremely low. However it does come with the unintentional affect that both will query to the same balance. https://test-insight.bitpay.com/address/2N1d4nqHgVUqY5HaUKmRpD69RMeigkYZKk4 https://test-insight.bitpay.com/address/moto6bxC99T4ZSj7F77izS1YrmpWQKxnGk |
|
@braydonf Low, but it does happen. And it appears there are some massive txs on the blockchain that intentionally target this, which is how I found it - one of my addresses has an incorrect balance because of this bug. |
|
I've just finished reindexing Testnet with this patch on our local instance - you have to wipe the leveldb directories due to the change in format. Balances are showing up correctly and the affected addresses are now fixed. |
|
Finished reindexing production, DB size is 54GB. Issue is fixed properly for us and all is running well. |
|
Finished syncing with testnet and can confirm that this fixes the issue. |
|
Aside from a few small nits, it looks good to me. Will also test with livenet soon. I think we should include this in the next major release, since even though a collision is improbable, it's possible to create unspendable collisions easily. We also add a command for reindexing (spawned an issue for that: #388). |
There was a problem hiding this comment.
nit: variable isn't used anymore
Prevents erroneous crediting of all transactions to both the p2pkh and the corresponding p2sh address.
8f651f0 to
3214390
Compare
|
@braydonf Fixed nits as above, added |
|
LGTM. I've been building from this branch for a week or more on livenet and testnet. |
|
👍 |
|
merging! |
Fix #377; db must contain hash + type, not just hash.
Prevents erroneous crediting of all transactions to both the
p2pkh and the corresponding p2sh address.
In order to do so, the DB must also contain the hash type. This will require a DB rebuild.
See #377