Documented bug in sign-extension behavior of opcodes OP_AND, OP_OR, and OP_XOR #1868
Conversation
…nd OP_XOR. Due to a bug in the implementation of MakeSameSize(), using OP_AND, OP_OR, or OP_XOR with signed values of unequal size will result in the sign-value becoming part of the smaller integer, with nonsensical results. This patch documents the unexpected behavior and provides the basis of a solution should decision be made to fix the bug in the future.
|
Good catch |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/95d7f002957a7bb30a2d5d8b07fe8fe3c1f37ec0 for binaries and test log. |
|
I don't like keeping dead code in source files, so I think the right thing to do is to remove MakeSameSize and the code for all of the disabled opcodes from the big switch() statement. If we ever resurrect them git will have the history, and, I assume, we'll write thorough unit tests to make sure they actually work correctly. So: I'd be ok with pulling this, and then another commit that removed code related to all the disabled opcodes. |
|
That's what I said on IRC: delete 'em. It is dead code that requires a hard fork to re-enable. |
|
Agree with Gavin, let's pull this as a warning to people if they'll ever resurrect this code, and then in a later pull remove all the code related to disabled opcodes. At the moment, they are only confusing. |
|
@laanwj sounds like a plan. |
|
I will create a new pull-request that surgically removes the disabled opcodes (they will now be caught by the EDIT: Sorry for the messy commit references surrounding this comment. Github refuses to forget about amended/removed commits :\ |
Documented bug in sign-extension behavior of opcodes OP_AND, OP_OR, and OP_XOR
|
Just a question... Isn't the proposed patch "wrong"? Negative numbers should be padded with 0xFF, not with 0. For example, in Little Endian, a 16 bit -1 is 0xFFFF, at 24 bits it's 0xFFFFFF, at 32 bits it's 0xFFFFFF. -2 at 16 bits is 0xFEFF, at 24 bits it's 0xFEFFFF... |
|
Bitcoin uses its own big-endian, explicit-bit semantics for BigNums. |
…s_wallet to obtain the collateral. 7f86031 wallet: GetMasternodeVinAndKeys was not locking cs_wallet to obtain the collateral tx. (furszy) Pull request description: Fixing `CWallet::GetMasternodeVinAndKeys` accessing `mapWallet` without locking `cs_wallet`. Decoupled from bitcoin#1829 . ACKs for top commit: random-zebra: utACK 7f86031 Fuzzbawls: utACK 7f86031 Tree-SHA512: 34995e3d65d6506d4a2465dd41b6d710d8a3e34f613a88691b6aa53018afa48a000faa444d03bbca0f418126fd5220a416d85a5a81d2053fd1412159b8cfb3c9
Due to a bug in the implementation of MakeSameSize(), using OP_AND, OP_OR, or OP_XOR with signed values of unequal size will result in the sign-bit becoming part of the smaller integer, with nonsensical results. This patch documents the unexpected behavior and provides the basis of a solution should decision be made to fix the bug in the future.