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.
Documented bug in sign-extension behavior of opcodes OP_AND, OP_OR, and OP_XOR #1868
pull maaku wants to merge 1 commits into bitcoin:master from maaku:and_or_xor_sign_extension changing 1 files +23 −1-
maaku commented at 11:46 PM on September 25, 2012: contributor
-
95d7f00295
Documented bug in sign-extension behavior of opcodes OP_AND, OP_OR, and 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.
-
laanwj commented at 7:37 AM on September 26, 2012: member
Good catch
-
BitcoinPullTester commented at 11:58 AM on September 26, 2012: none
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/95d7f002957a7bb30a2d5d8b07fe8fe3c1f37ec0 for binaries and test log.
-
gavinandresen commented at 2:36 PM on September 26, 2012: contributor
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.
-
jgarzik commented at 3:11 PM on September 26, 2012: contributor
That's what I said on IRC: delete 'em. It is dead code that requires a hard fork to re-enable.
-
laanwj commented at 4:59 PM on September 26, 2012: member
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.
-
-
maaku commented at 5:22 PM on September 26, 2012: contributor
I will create a new pull-request that surgically removes the disabled opcodes (they will now be caught by the
default: return falsecatch-all).EDIT: Sorry for the messy commit references surrounding this comment. Github refuses to forget about amended/removed commits :\
- laanwj referenced this in commit 035cb4781d on Sep 28, 2012
- laanwj merged this on Sep 28, 2012
- laanwj closed this on Sep 28, 2012
-
xanatos commented at 6:05 AM on April 26, 2013: none
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...
-
maaku commented at 4:48 PM on April 26, 2013: contributor
Bitcoin uses its own big-endian, explicit-bit semantics for BigNums.
- KolbyML referenced this in commit fb94ca77d4 on Dec 5, 2020
- DrahtBot locked this on Sep 8, 2021
