Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Jan 31, 2022

Change the tapscript validation weight constants from uint64_t to int64_t, since the type of m_validation_weight_left is also int64_t. Otherwise this will cause sanitizer warnings.

This should be safe because signed integer overflow isn't expected to happen.

@maflcko
Copy link
Member Author

maflcko commented Jan 31, 2022

On my system, this commit doesn't change the binary for gcc with O2, but it does change it for clang

Copy link
Contributor

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

utACK fadc54b

@maflcko
Copy link
Member Author

maflcko commented Feb 1, 2022

centos 8 CI failure can be ignored

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK fadc54b

For reference, these are the only instances where the constants VALIDATION_WEIGHT_{PER_SIGOP_PASSED,OFFSET} are used:

execdata.m_validation_weight_left -= VALIDATION_WEIGHT_PER_SIGOP_PASSED;

execdata.m_validation_weight_left = ::GetSerializeSize(witness.stack, PROTOCOL_VERSION) + VALIDATION_WEIGHT_OFFSET;

Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK

@maflcko maflcko merged commit 5034b7f into bitcoin:master Feb 7, 2022
@maflcko maflcko deleted the 2201-tapscriptInt branch February 7, 2022 08:29
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 7, 2022
…ation weight calculation

fadc54b Fix unsigned integer overflow in tapscript validation weight calculation (MarcoFalke)

Pull request description:

  Change the tapscript validation weight constants from uint64_t to int64_t, since the type of m_validation_weight_left is also int64_t. Otherwise this will cause sanitizer warnings.

  This should be safe because signed integer overflow isn't expected to happen.

ACKs for top commit:
  PastaPastaPasta:
    utACK fadc54b
  theStack:
    Code-review ACK fadc54b

Tree-SHA512: 7a62d3a84733ab7827e3fa50d83f5493f2481b725c587e986eb2c128a769f023756f3ad964401526e386a847aa630a9f6c43a57d25ce5fd4af0b6bb5e0615528
@bitcoin bitcoin locked and limited conversation to collaborators Feb 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants