Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OP_INVERT was modifying bytes in place, now they are popped, modified, and pushed back onto the stack #169

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

sirdeggen
Copy link
Contributor

@sirdeggen sirdeggen commented Aug 15, 2023

Seems like it's only OP_INVERT which was causing this strange bug.

Scenario

PushTx methodology is used across a number of smart contracts. Optimized code uses a public key associated with the private key Bn(1). The unlocking script contains a complete preimage for the sake of forward enforcement among other things. That preimage has its nSequence inverted somewhere in the example script, but while it's inverted on the stack, the other items on the stack containing the same bytes are also inverted.

Fix

Rather than Peek, we Pop the byte array, clone it, invert the clone and push that on to the stack. It might not be as memory efficient, but it at leasts solves this bizarre problem.

This is where the error is being generated. Maybe we can fix lower in the stack if we think about split?

Signed-off-by: Darren Kellenschwiler <deggen@kschw.com>
@sirdeggen sirdeggen self-assigned this Aug 15, 2023
@sirdeggen sirdeggen added bug Something isn't working and removed bug-P3 labels Aug 15, 2023
@mergify mergify bot added the bug-P3 label Aug 15, 2023
@codecov-commenter
Copy link

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (51c16ed) 80.63% compared to head (4df7aed) 80.64%.
Report is 1 commits behind head on master.

❗ Current head 4df7aed differs from pull request most recent head e45ae11. Consider uploading reports for the commit e45ae11 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #169   +/-   ##
=======================================
  Coverage   80.63%   80.64%           
=======================================
  Files          38       38           
  Lines        4115     4117    +2     
=======================================
+ Hits         3318     3320    +2     
  Misses        546      546           
  Partials      251      251           
Files Changed Coverage Δ
bscript/interpreter/operations.go 96.55% <100.00%> (+<0.01%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sirdeggen sirdeggen changed the title Script Operations were modifying bytes in place, now they are popped, modified, and pushed back on to the stack. OP_INVERT was modifying bytes in place, now they are popped, modified, and pushed back onto the stack Aug 15, 2023
Signed-off-by: Darren Kellenschwiler <deggen@kschw.com>

# Conflicts:
#	bscript/interpreter/engine_test.go
@sirdeggen sirdeggen merged commit 43ccba7 into master Aug 16, 2023
14 checks passed
@sirdeggen sirdeggen deleted the fix/op-invert-bug-fix branch August 16, 2023 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working bug-P3
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants