-
Notifications
You must be signed in to change notification settings - Fork 641
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix #377; db must contain hash + type, not just hash. #379
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. |
hashTypeBuffer: hashTypeBuffer, | ||
hashTypeReadable: addrObj.type | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing semicolon
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). |
@@ -698,6 +732,26 @@ AddressService.prototype._decodeInputValueMap = function(buffer) { | |||
}; | |||
}; | |||
|
|||
AddressService.prototype._getAddressInfo = function(addressStr) { | |||
var addrObj = bitcore.Address(addressStr); | |||
var hashbuffer = addrObj.hashBuffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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