mastercoin-MSC / mastercore

mastercore info
mastercoin.org
MIT License
24 stars 11 forks source link

Update to Bitcoin Core 0.9.4 #274

Closed dexX7 closed 9 years ago

dexX7 commented 9 years ago

In before: most changes are related to translations and it's easier to see, when looking at the commits.

This is an update to Bitcoin Core 0.9.4, which is consensus critical and mandatory under some circumstances.

OpenSSL 1.0.0p / 1.0.1k was recently released and is being pushed out by various operating system maintainers. Review by Gregory Maxwell determined that this update is incompatible with the Bitcoin system and could lead to consensus forks. The incompatibility is due to the OpenSSL update changing the behavior of ECDSA validation to reject any signature which is not encoded in a very rigid manner. This was a result of OpenSSL's change for CVE-2014-8275 "Certificate fingerprints can be modified".

To check, if you are (currently) affected: run the tests with ./src/test/test_bitcoin. It results in 4 test failures of transaction_tests.cpp.

This is relevant for current systems such as Ubuntu 14.04 with OpenSSL updated in 2015 (this is part of the recommended OS updates).

See: http://sourceforge.net/p/bitcoin/mailman/message/33221963/ https://bitcointalk.org/index.php?topic=919373.0

flaneur2020 commented 9 years ago
mastercoin@mmastercoind:~/build/mastercore$ src/test/test_bitcoin
mastercore_init(), line 2434, file: mastercore.cpp
CMPTradeList(): OK, line 376, file: mastercore.h
CMPSTOList(): OK, line 333, file: mastercore.h
CMPTxList(): OK, line 433, file: mastercore.h
[Snapshot] Exodus balance: 0.00000000
[Initialized] Exodus balance: 0.00000000
Running 132 test cases...
transaction_tests.cpp(91): error in "tx_valid": [[["60a20bd93aa49ab4b28d514ec10b06e1829ce6818ec06cd3aabd013ebcdc4bb1",0,"1 0x41 0x04cc71eb30d653c0c3163990c47b976f3fb3f37cccdcbedb169a1dfef58bbfbfaff7d8a473e7e2e6d317b87bafe8bde97e3cf8f065dec022b51d11fcdd0d348ac4 0x41 0x0461cbdcc5409fb4b4d42b51d33381354d80e550078cb532a34bfa2fcfdeb7d76519aecc62770f5b0e4ef8551946d8a540911abe3e7854a26f39f58b25c15342af 2 OP_CHECKMULTISIG"]],"0100000001b14bdcbc3e01bdaad36cc08e81e69c82e1060bc14e518db2b49aa43ad90ba26000000000490047304402203f16c6f40162ab686621ef3000b04e75418a0c0cb2d8aebeac894ae360ac1e780220ddc15ecdfc3507ac48e1681a33eb60996631bf6bf5bc0a0682c4db743ce7ca2b01ffffffff0140420f00000000001976a914660d4ef3a743e3e696ad990364e555c271ad504b88ac00000000",true]
transaction_tests.cpp(91): error in "tx_valid": [[["60a20bd93aa49ab4b28d514ec10b06e1829ce6818ec06cd3aabd013ebcdc4bb1",0,"1 0x41 0x04cc71eb30d653c0c3163990c47b976f3fb3f37cccdcbedb169a1dfef58bbfbfaff7d8a473e7e2e6d317b87bafe8bde97e3cf8f065dec022b51d11fcdd0d348ac4 0x41 0x0461cbdcc5409fb4b4d42b51d33381354d80e550078cb532a34bfa2fcfdeb7d76519aecc62770f5b0e4ef8551946d8a540911abe3e7854a26f39f58b25c15342af 2 OP_CHECKMULTISIG"]],"0100000001b14bdcbc3e01bdaad36cc08e81e69c82e1060bc14e518db2b49aa43ad90ba260000000004A0048304402203f16c6f40162ab686621ef3000b04e75418a0c0cb2d8aebeac894ae360ac1e780220ddc15ecdfc3507ac48e1681a33eb60996631bf6bf5bc0a0682c4db743ce7ca2bab01ffffffff0140420f00000000001976a914660d4ef3a743e3e696ad990364e555c271ad504b88ac00000000",true]
transaction_tests.cpp(91): error in "tx_valid": [[["b464e85df2a238416f8bdae11d120add610380ea07f4ef19c5f9dfd472f96c3d",0,"DUP HASH160 0x14 0xbef80ecf3a44500fda1bc92176e442891662aed2 EQUALVERIFY CHECKSIG"],["b7978cc96e59a8b13e0865d3f95657561a7f725be952438637475920bac9eb21",1,"DUP HASH160 0x14 0xbef80ecf3a44500fda1bc92176e442891662aed2 EQUALVERIFY CHECKSIG"]],"01000000023d6cf972d4dff9c519eff407ea800361dd0a121de1da8b6f4138a2f25de864b4000000008a4730440220ffda47bfc776bcd269da4832626ac332adfca6dd835e8ecd83cd1ebe7d709b0e022049cffa1cdc102a0b56e0e04913606c70af702a1149dc3b305ab9439288fee090014104266abb36d66eb4218a6dd31f09bb92cf3cfa803c7ea72c1fc80a50f919273e613f895b855fb7465ccbc8919ad1bd4a306c783f22cd3227327694c4fa4c1c439affffffff21ebc9ba20594737864352e95b727f1a565756f9d365083eb1a8596ec98c97b7010000008a4730440220503ff10e9f1e0de731407a4a245531c9ff17676eda461f8ceeb8c06049fa2c810220c008ac34694510298fa60b3f000df01caa244f165b727d4896eb84f81e46bcc4014104266abb36d66eb4218a6dd31f09bb92cf3cfa803c7ea72c1fc80a50f919273e613f895b855fb7465ccbc8919ad1bd4a306c783f22cd3227327694c4fa4c1c439affffffff01f0da5200000000001976a914857ccd42dded6df32949d4646dfa10a92458cfaa88ac00000000",true]
transaction_tests.cpp(91): error in "tx_valid": [[["b464e85df2a238416f8bdae11d120add610380ea07f4ef19c5f9dfd472f96c3d",0,"DUP HASH160 0x14 0xbef80ecf3a44500fda1bc92176e442891662aed2 EQUALVERIFY CHECKSIG"],["b7978cc96e59a8b13e0865d3f95657561a7f725be952438637475920bac9eb21",1,"DUP HASH160 0x14 0xbef80ecf3a44500fda1bc92176e442891662aed2 EQUALVERIFY CHECKSIG"]],"01000000023d6cf972d4dff9c519eff407ea800361dd0a121de1da8b6f4138a2f25de864b4000000008a4730440220ffda47bfc776bcd269da4832626ac332adfca6dd835e8ecd83cd1ebe7d709b0e022049cffa1cdc102a0b56e0e04913606c70af702a1149dc3b305ab9439288fee090014104266abb36d66eb4218a6dd31f09bb92cf3cfa803c7ea72c1fc80a50f919273e613f895b855fb7465ccbc8919ad1bd4a306c783f22cd3227327694c4fa4c1c439affffffff21ebc9ba20594737864352e95b727f1a565756f9d365083eb1a8596ec98c97b7010000008a4730440220503ff10e9f1e0de731407a4a245531c9ff17676eda461f8ceeb8c06049fa2c810220c008ac34694510298fa60b3f000df01caa244f165b727d4896eb84f81e46bcc4014104266abb36d66eb4218a6dd31f09bb92cf3cfa803c7ea72c1fc80a50f919273e613f895b855fb7465ccbc8919ad1bd4a306c783f22cd3227327694c4fa4c1c439affffffff01f0da5200000000001976a914857ccd42dded6df32949d4646dfa10a92458cfaa88ac00000000",true]

*** 4 failures detected in test suite "Bitcoin Test Suite"

ubuntu 14.04 with the latest libssl-dev, 4 test failures of transaction_tests.cpp exactly.

thank you!

zathras-crypto commented 9 years ago

Right thanks DexX. I took a quick look at the changes and whilst it seems large in scope initially most of it is just translations which isn't too much of a concern. I couldn't see anything that would be problematic, the only thing that raised an eyebrow was what looks like disabling of SSLv3 - not sure why that's done but regardless I think it's pretty unlikely any of our integrators are forcing a specific SSL version on their RPC connections.

@m21 this is a bit of a different method to the way you usually do updates to the underlying bitcoin version but this does seem viable to just pull in the changes DexX has PR'd instead? Any reasons not to merge?

Thanks Z

dexX7 commented 9 years ago

Regarding SSL, it looks like this is the commit, and it has some explanation.

0a94661e8db94e84ecbf1ea45a51fb3c7fb77283:

TLS is subject to downgrade attacks when SSLv3 is available, and
 SSLv3 has vulnerabilities.

The popular solution is to disable SSLv3. On the web this breaks
 some tiny number of very old clients. While Bitcoin RPC shouldn't
 be exposed to the open Internet, it also shouldn't be exposed to
 really old SSL implementations, so it shouldn't be a major issue
 for us to disable SSLv3.

There is more information on the downgrade attacks and disabling
 SSLv3 at https://disablessl3.com/ .

To compare the overall results, you may confirm the changes by following the same route:

git remote add upstream https://github.com/mastercoin-MSC/mastercore.git
git remote add bitcoin https://github.com/bitcoin/bitcoin.git
git fetch bitcoin 0.9
git fetch upstream mscore-0.0.9
git checkout -b mscore-0.0.9.4 upstream/mscore-0.0.9
git pull bitcoin 0.9

There will be three trivial merge conflicts:

alert.h (I used HEAD):

<<<<<<< HEAD
    bool ProcessAlert(bool fThread = true); // fThread means run -alertnotify in a free-running thread
=======
    bool ProcessAlert(bool fThread = true);
>>>>>>> 41f94edf221019a6c4b8f78c2b17c389442f546e

main.cpp (I used 41f94e):

<<<<<<< HEAD
                   pindexBestForkBase->phashBlock->ToString() + std::string("'");
            CAlert::Notify(warning, true);        }
=======
                pindexBestForkBase->phashBlock->ToString() + std::string("'");
            CAlert::Notify(warning, true);
        }
>>>>>>> 41f94edf221019a6c4b8f78c2b17c389442f546e

README.md (I used HEAD):

<<<<<<< HEAD
http://www.mastercoin.org
=======
Copyright (c) 2009-2015 Bitcoin Core Developers
>>>>>>> 41f94edf221019a6c4b8f78c2b17c389442f546e

Once resolved:

git add README.md src/alert.h src/main.cpp
git commit -S

The result should be identical to this PR.

m21 commented 9 years ago

Just the number of changed files scares me -- even if it were Satoshi himself coming in with 55 changes just before we are doing a tag. :) I checked and the 3 consensus-affecting fixes are within 30 lines of code: https://github.com/bitcoin/bitcoin/commit/037bfefe6bccbdf656e628a1f4526db8f80c3922 https://github.com/bitcoin/bitcoin/commit/60c51f1c381bbd93c70cfdf41c6688609a7956fc https://github.com/bitcoin/bitcoin/commit/b8e81b7ccd4490155e3345fc73346ff8c3a77524

So perhaps pull just them in?

dexX7 commented 9 years ago

So perhaps pull just them in?

Hehe, yeah most of it appears to be related to translations and Gitian building. But I can confirm, those three commits seem to be sufficient to solve all test failures. Care should be taken, when build via Gitian though.

even if it were Satoshi himself coming in with 55 changes just before we are doing a tag. :)

It's always before the next tag. :P

flaneur2020 commented 9 years ago
mastercoin@mmastercore2:~$ mastercore/src/test/test_bitcoin
mastercore_init(), line 2434, file: mastercore.cpp
CMPTradeList(): OK, line 376, file: mastercore.h
CMPSTOList(): OK, line 333, file: mastercore.h
CMPTxList(): OK, line 433, file: mastercore.h
[Snapshot] Exodus balance: 0.00000000
[Initialized] Exodus balance: 0.00000000
Running 132 test cases...

*** No errors detected

no error detected after patching this PR

regards

m21 commented 9 years ago

"always before the tag" -- so true :) Anyhow, here's the cherry-picked ECDSA fix: https://github.com/mastercoin-MSC/mastercore/commit/bde4874142b6d1e7c40db2e2d21fa5308d5e85fb thanks Dexx, Fleurer, Zathras (I hope we can now skip 0.9.4 and go directly to Bitcoin 10).