jimmysong / programmingbitcoin

Repository for the book
Other
1.75k stars 656 forks source link

Potential Safety Issue On Script Execution Success Determination #285

Open salmonberry7 opened 1 month ago

salmonberry7 commented 1 month ago

In the function Script.evaluate on p116 and in line 134 in script.py at : Chapter 6, script.py would it not be better to have : if decode_num(stack.pop()) == 0: instead of : if stack.pop() == b'': since the functions decode_num and encode_num in op.py already encapsulate the mapping between byte vectors on the stack and signed integers as described in Bitcoin Wiki. decode_num already has the test if element == b'': return 0. Other representations of zero are possible besides the null length vector, eg 0x00 (1 zero byte), 0x00 0x00 (2 zero bytes), and 0x80 ('minus zero'). It is maybe best to handle all of these within decode_num to keep them all in the one place. If any of these other representations of zero ended up at the top of the stack on completion of the script then Script.evaluate would currently return True and then Tx.verify_input would return True, thus validating an input that maybe should not be considered valid. Not sure if there are any corner cases where this could arise, but maybe a good idea to guard against them anyway - defensive coding. At least it would be tidier to put it through decode_num.