work towards bringing mininode.py up to date with the main node #256
Conversation
There was a problem hiding this comment.
previously this function would not produce the canonical encoding for values [1, 16]
There was a problem hiding this comment.
Not a script expert, but these values correspond to opcodes, not the resulting value pushed on the stack.
The code as-is matches the C++ code quite well.
There was a problem hiding this comment.
this function is used in exactly one place (line 69 to the left). Its result is passed to ser_string() and passed in as a script. Perhaps I'm missing something, but it's not clear to me how that could possibly generate a valid script (apart from the special case of 0 and possibly some others). This check was failing in the test.
There was a problem hiding this comment.
that said, it's probably better to leave this function as-is, and instead wrap it in CScript() where it's used instead
There was a problem hiding this comment.
it also would not include the length-prefix
There was a problem hiding this comment.
Again, I don't think is required. If someone is pushing the number onto the stack, they will append this value.
There was a problem hiding this comment.
I'm not sure what the most reasonable default value for asset is. Should it be another parameter?
There was a problem hiding this comment.
It's likely going the be whatever policyAsset is set to, in most cases "bitcoin"'s asset type.
There was a problem hiding this comment.
this comment refers to this condition. Come to think of it, perhaps that check is incorrect. It seems to assume that the height at the start of the script must use a two-byte encoding (which is only true for values > 16)
There was a problem hiding this comment.
you mean the comment or the patch to make the scriptsig at least 2 bytes? I have a feeling that this requirement of the scriptsig being at least 2 bytes is a mistake. But I can't say I understand the motivation of it well enough to be confident. I imagine it's not a problem in the wild, as the block height most likely will get encoded as at least two bytes in the script. But in tests where the height < 16, it is.
There was a problem hiding this comment.
the C++ version of these 3 classes are a single template. Perhaps these should be merged as well and use variables instead of template parameters.
There was a problem hiding this comment.
the serialize_with_witness function was duplicated. This removes the second copy as the first one matched the C++ code better
arvidn
force-pushed
the
mininode-test-update
branch
from
ba52323 to
ab722bb
Compare
|
seems you left some random net_processing.cpp changes, please remove. |
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
thanks for working on this!
A large part missing here is how the witness structure itself is serialized, and issuances.
There was a problem hiding this comment.
Put a TODO to check block header sigs?
There was a problem hiding this comment.
Nonce class only has a 33-byte commitment, not a rangeproof or other witness data.
There was a problem hiding this comment.
rangeproof no longer resides in the output. It resides in the CTxWitness based field wit in CTransaction
There was a problem hiding this comment.
It's likely going the be whatever policyAsset is set to, in most cases "bitcoin"'s asset type.
There was a problem hiding this comment.
Not a script expert, but these values correspond to opcodes, not the resulting value pushed on the stack.
The code as-is matches the C++ code quite well.
There was a problem hiding this comment.
Again, I don't think is required. If someone is pushing the number onto the stack, they will append this value.
There was a problem hiding this comment.
Rangeproof is in witness structure.
There was a problem hiding this comment.
it's not a nonce, just an asset commitment
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
Feel free to squash liberally
There was a problem hiding this comment.
copy and paste error, these should be range/surj proofs
There was a problem hiding this comment.
scriptWitness is for inputs
There was a problem hiding this comment.
scriptWitness is for inputs
There was a problem hiding this comment.
scriptWitness is for inputs (below as well)
…owards re-enabling rpc tests
arvidn
force-pushed
the
mininode-test-update
branch
from
fd81ec6 to
3c21411
Compare
|
did some work on top: https://github.com/instagibbs/elements/commits/mininode re-activated a test that uses mininode transaction serialization(without any witness data). |
|
tACK 3c21411, got the entire test working. |
…n node 3c21411 work towards bringing mininode.py up to date with the main node and towards re-enabling rpc tests (Arvid Norberg)
3 participants
and towards re-enabling rpc tests. This patch introduces
CProof,CTxOutValue,CTxOutAssetandCTxOutNonceto mininode.py. It also adds some debug output to the main node that was helpful in trouble-shooting the interaction between mininode.py and the main node.This patch is a step in the direction of having mininode be compatible with elements. it does not bring it nor the tests all the way there to pass.