Skip to content
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

Merged
merged 1 commit into from
Jan 11, 2016

Conversation

STRML
Copy link
Contributor

@STRML STRML commented Dec 18, 2015

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

@STRML
Copy link
Contributor Author

STRML commented Dec 18, 2015

Note that this changes some internal APIs.

getInputs() and getOutputs() haven't changed, so there shouldn't be any breakage in insight-api, but the arity of _getInputsMempool() and _getOutputsMempool() has changed to include the hashTypeBuffer.

Additionally, getInputs() and getOutputs() now yield objects with a hashType string, which is identical to the strings used in bitcore.Address.

@STRML STRML force-pushed the bugfix/hashType branch 2 times, most recently from f4898f4 to 470727c Compare December 18, 2015 16:38
@STRML
Copy link
Contributor Author

STRML commented Dec 18, 2015

Here's an easy (low block height) address that shows an erroneous balance: https://test-insight.bitpay.com/address/2N1d4nqHgVUqY5HaUKmRpD69RMeigkYZKk4

@STRML STRML force-pushed the bugfix/hashType branch 2 times, most recently from 6d8fc91 to 8f651f0 Compare December 18, 2015 19:02
@STRML
Copy link
Contributor Author

STRML commented Dec 18, 2015

Rebased due to a test error in _getOutputsMempool, all tests green now.

@braydonf
Copy link
Contributor

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

{ 
  hash: '5be27ad49c77a1fe275e59a02f79be1ce9aaa430',
  type: 'scripthash',
  network: 'testnet' 
}

https://test-insight.bitpay.com/address/moto6bxC99T4ZSj7F77izS1YrmpWQKxnGk

{ 
  hash: '5be27ad49c77a1fe275e59a02f79be1ce9aaa430',
  type: 'pubkeyhash',
  network: 'testnet' 
}

@STRML
Copy link
Contributor Author

STRML commented Dec 18, 2015

@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.

@STRML
Copy link
Contributor Author

STRML commented Dec 18, 2015

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.

@STRML STRML changed the title Fix #377; db must contain hash type, not just hash. Fix #377; db must contain hash + type, not just hash. Dec 18, 2015
@STRML
Copy link
Contributor Author

STRML commented Dec 22, 2015

Finished reindexing production, DB size is 54GB. Issue is fixed properly for us and all is running well.

@braydonf
Copy link
Contributor

Finished syncing with testnet and can confirm that this fixes the issue.

hashTypeBuffer: hashTypeBuffer,
hashTypeReadable: addrObj.type
};
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing semicolon

@braydonf
Copy link
Contributor

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;
Copy link
Contributor

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.
@STRML
Copy link
Contributor Author

STRML commented Jan 11, 2016

@braydonf Fixed nits as above, added HASH_TYPE_MAP per your suggestion.

@braydonf
Copy link
Contributor

LGTM. I've been building from this branch for a week or more on livenet and testnet.

@STRML
Copy link
Contributor Author

STRML commented Jan 11, 2016

👍 :shipit:!

@kleetus
Copy link
Contributor

kleetus commented Jan 11, 2016

merging!

kleetus added a commit that referenced this pull request Jan 11, 2016
Fix #377; db must contain hash + type, not just hash.
@kleetus kleetus merged commit 7931062 into bitpay:master Jan 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants