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

[RPC][Bitcoin-TX] Add support for sequence number #7957

Merged
merged 3 commits into from Jun 7, 2016

Conversation

Projects
None yet
5 participants
@jonasschnelli
Member

jonasschnelli commented Apr 27, 2016

No description provided.

jonasschnelli added some commits Dec 22, 2015

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Apr 27, 2016

Member

Subset PR as requested in #7865.

Member

jonasschnelli commented Apr 27, 2016

Subset PR as requested in #7865.

const UniValue& sequenceObj = find_value(o, "sequence");
if (sequenceObj.isNum())
nSequence = sequenceObj.get_int();

This comment has been minimized.

@mruddy

mruddy Apr 28, 2016

Contributor

Neat, this is useful. I was trying it out and think that being able to use hex is a little easier. Maybe something like this:

if (sequenceObj.isNum())
{
    nSequence = sequenceObj.get_int();
} else if (sequenceObj.isStr() /*&& IsHex(sequenceObj.get_str())*/)
{
    nSequence = strtoul(sequenceObj.get_str().c_str(), NULL, 16);
}

I commented out the IsHex check because I think it might be too restrictive (but left it there to see what you thought).
With this little patch, these work too:

createrawtransaction '[{"txid":"d5eb7e43a7cf85c5bee47a7a33c8351b8dcc013616a7c6bdc3af49538d986221","vout":0,"sequence":"0xfffffffe"}]' '{"mifzFw94bSGApmx2eWw1nzLCfEeq4cTwey":49.99}' 0
createrawtransaction '[{"txid":"d5eb7e43a7cf85c5bee47a7a33c8351b8dcc013616a7c6bdc3af49538d986221","vout":0,"sequence":"fffffffd"}]' '{"mifzFw94bSGApmx2eWw1nzLCfEeq4cTwey":49.99}' 0
createrawtransaction '[{"txid":"d5eb7e43a7cf85c5bee47a7a33c8351b8dcc013616a7c6bdc3af49538d986221","vout":0,"sequence":"f"}]' '{"mifzFw94bSGApmx2eWw1nzLCfEeq4cTwey":49.99}' 0
@mruddy

mruddy Apr 28, 2016

Contributor

Neat, this is useful. I was trying it out and think that being able to use hex is a little easier. Maybe something like this:

if (sequenceObj.isNum())
{
    nSequence = sequenceObj.get_int();
} else if (sequenceObj.isStr() /*&& IsHex(sequenceObj.get_str())*/)
{
    nSequence = strtoul(sequenceObj.get_str().c_str(), NULL, 16);
}

I commented out the IsHex check because I think it might be too restrictive (but left it there to see what you thought).
With this little patch, these work too:

createrawtransaction '[{"txid":"d5eb7e43a7cf85c5bee47a7a33c8351b8dcc013616a7c6bdc3af49538d986221","vout":0,"sequence":"0xfffffffe"}]' '{"mifzFw94bSGApmx2eWw1nzLCfEeq4cTwey":49.99}' 0
createrawtransaction '[{"txid":"d5eb7e43a7cf85c5bee47a7a33c8351b8dcc013616a7c6bdc3af49538d986221","vout":0,"sequence":"fffffffd"}]' '{"mifzFw94bSGApmx2eWw1nzLCfEeq4cTwey":49.99}' 0
createrawtransaction '[{"txid":"d5eb7e43a7cf85c5bee47a7a33c8351b8dcc013616a7c6bdc3af49538d986221","vout":0,"sequence":"f"}]' '{"mifzFw94bSGApmx2eWw1nzLCfEeq4cTwey":49.99}' 0

This comment has been minimized.

@jonasschnelli

jonasschnelli Apr 28, 2016

Member

Thanks for the review!
Hmm... I'm not sure if we want utility function (hex->int) on machine-to-machine communication (RPC).
But no strong opinion.

@jonasschnelli

jonasschnelli Apr 28, 2016

Member

Thanks for the review!
Hmm... I'm not sure if we want utility function (hex->int) on machine-to-machine communication (RPC).
But no strong opinion.

This comment has been minimized.

@mruddy

mruddy Apr 28, 2016

Contributor

Good point, I was thinking of only the GUI RPC console use (that's where the hex conversion is useful to me).
If your threat model includes hostile input on this interface, then the IsHex check could be used and the length of the string could be validated to be <= 8. That would likely be sufficient input validation. Adding back IsHex makes my first and third examples invalid. The sequence value of the third would need to change to "0f" instead of just "f".

@mruddy

mruddy Apr 28, 2016

Contributor

Good point, I was thinking of only the GUI RPC console use (that's where the hex conversion is useful to me).
If your threat model includes hostile input on this interface, then the IsHex check could be used and the length of the string could be validated to be <= 8. That would likely be sufficient input validation. Adding back IsHex makes my first and third examples invalid. The sequence value of the third would need to change to "0f" instead of just "f".

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 2, 2016

Member

utACK e59336f

Member

sipa commented Jun 2, 2016

utACK e59336f

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 6, 2016

Contributor

utACK e59336f

Contributor

paveljanik commented Jun 6, 2016

utACK e59336f

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jun 7, 2016

Member

Added a commit with two bitcoin-tx tests to cover the sequence number feature.

Member

jonasschnelli commented Jun 7, 2016

Added a commit with two bitcoin-tx tests to cover the sequence number feature.

@laanwj laanwj merged commit ae357d5 into bitcoin:master Jun 7, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

laanwj added a commit that referenced this pull request Jun 7, 2016

Merge #7957: [RPC][Bitcoin-TX] Add support for sequence number
ae357d5 [Bitcoin-Tx] Add tests for sequence number support (Jonas Schnelli)
e59336f [bitcoin-tx] allow to set nSequence number over the in= command (Jonas Schnelli)
a946bb6 [RPC] createrawtransaction: add option to set the sequence number per input (Jonas Schnelli)
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 7, 2016

Member

utACK ae357d5

Member

laanwj commented Jun 7, 2016

utACK ae357d5

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7957: [RPC][Bitcoin-TX] Add support for sequence number
ae357d5 [Bitcoin-Tx] Add tests for sequence number support (Jonas Schnelli)
e59336f [bitcoin-tx] allow to set nSequence number over the in= command (Jonas Schnelli)
a946bb6 [RPC] createrawtransaction: add option to set the sequence number per input (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7957: [RPC][Bitcoin-TX] Add support for sequence number
ae357d5 [Bitcoin-Tx] Add tests for sequence number support (Jonas Schnelli)
e59336f [bitcoin-tx] allow to set nSequence number over the in= command (Jonas Schnelli)
a946bb6 [RPC] createrawtransaction: add option to set the sequence number per input (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #7957: [RPC][Bitcoin-TX] Add support for sequence number
ae357d5 [Bitcoin-Tx] Add tests for sequence number support (Jonas Schnelli)
e59336f [bitcoin-tx] allow to set nSequence number over the in= command (Jonas Schnelli)
a946bb6 [RPC] createrawtransaction: add option to set the sequence number per input (Jonas Schnelli)

@str4d str4d referenced this pull request Mar 14, 2018

Merged

CLI binary improvements #3086

zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018

Auto merge of #3086 - str4d:cli-binary-improvements-1, r=str4d
CLI binary improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5936
- bitcoin/bitcoin#7550
- bitcoin/bitcoin#7989
- bitcoin/bitcoin#7957
- bitcoin/bitcoin#9067
- bitcoin/bitcoin#9220

Excludes any changes that affected the QT code.

zkbot added a commit to zcash/zcash that referenced this pull request Apr 13, 2018

Auto merge of #3086 - str4d:cli-binary-improvements-1, r=str4d
CLI binary improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#5936
- bitcoin/bitcoin#7550
- bitcoin/bitcoin#7989
- bitcoin/bitcoin#7957
- bitcoin/bitcoin#9067
- bitcoin/bitcoin#9220

Excludes any changes that affected the QT code.

sickpig added a commit to sickpig/BitcoinUnlimited that referenced this pull request May 4, 2018

Port XT PR #403: [RPC][Bitcoin-TX] Add support for sequence number
bitcoin/bitcoin#7957 - [RPC][Bitcoin-TX] Add support for sequence number
bitcoin/bitcoin#8164 - [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue
bitcoin/bitcoin#8171 - [RPC] Fix createrawtx sequence number unsigned int parsing

[RPC][Bitcoin-TX] Add support for sequence number

sickpig added a commit to sickpig/BitcoinUnlimited that referenced this pull request May 9, 2018

Port XT PR #403: [RPC][Bitcoin-TX] Add support for sequence number
bitcoin/bitcoin#7957 - [RPC][Bitcoin-TX] Add support for sequence number
bitcoin/bitcoin#8164 - [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue
bitcoin/bitcoin#8171 - [RPC] Fix createrawtx sequence number unsigned int parsing

[RPC][Bitcoin-TX] Add support for sequence number
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment