Conversation
|
Hey! Great to see some interest on the project, I'll try to take the time to review this soon. Reach me on Discord (or propose other channel) if you want to talk Discord UserID: fr0dg3r |
Jonatanc05
left a comment
Jonatanc05
left a comment
There was a problem hiding this comment.
Nice job with the implementation. Some changes pointed.
Also, could you write a test for this opcode? You could use example values in tools like https://learnmeabitcoin.com/technical/script/p2ms/
You can make a test as simple as the existing test "script: Basic script execution"
src/bitcoin.zig
Outdated
| } | ||
|
|
||
| const num_sigs: i64 = stack.popInt() catch return error.BadScript; | ||
| if (num_sigs > MAX_SIGNATURES) { return error.BadScript; } |
There was a problem hiding this comment.
We should also prevent num_sigs from exceeding num_keys
src/bitcoin.zig
Outdated
| // see https://github.com/bitcoin/bips/blob/master/bip-0147.mediawiki | ||
| const dummy_buf: [1]u8 = undefined; | ||
| stack.pop(dummy_buf) catch |err| return Local.handlePopError(err); | ||
| if (dummy_buf[0] != Op.OP_0) return error.BadScript; |
There was a problem hiding this comment.
We should actually assign the returned value of stack.pop
The way I wrote stack.pop is to use the returned slice, which is subset of the buffer sent. In this case, though, we would be reading garbage on dummy_buf[0]. What we actually want is check that slice returned by pop has len == 0
- It's a shame that we don't get to read OP_0 explicitly, but the comment makes clear what's happening.
- It's also a shame that the compiler doesn't force us to assign the return value of
stack.pop... that's not what I expected (in the general case, it's a compile error to implicitly ignore a return value, maybe catching errors changes that for some reason)
src/bitcoin.zig
Outdated
| for (pubkeys) |pubkey| { | ||
| const signature = signatures[num_valid_sigs]; | ||
| if (try Tx.checksig(transaction.?, input_index.?, pubkey, signature, script, alloc)) { | ||
| num_valid_sigs += 1; | ||
| } | ||
| if (num_valid_sigs == num_sigs) { break; } | ||
| } |
There was a problem hiding this comment.
I am a little tired to think about wether it makes a difference or not... but we should scan all pubkeys for each signature instead of scanning all signatures for each pubkey. That might have performance impact, not sure if we would get different output.
Also we could exit early when the number of remaining pubkeys is less the number of remaning signatures to be checked. In that case, we know we'll fail. See bitcoin\src\script\interpreter.cpp
There was a problem hiding this comment.
The variable name is a bit misleading but num_valid_sigs is actually used as an index into which signature we're checking. Keys are guaranteed to be ordered, and they're not checked repeatedly for each signature.
I changed it to return early when there's not enough keys left
3 participants
This is my first time writing Zig, please lmk if I need to change anything. I plan to work on other multisig stuff soon