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

[Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue #8164

Merged
merged 1 commit into from Jun 8, 2016

Conversation

Projects
None yet
4 participants
@jonasschnelli
Member

jonasschnelli commented Jun 7, 2016

Travis is currently failing because of missing test fixtures from #7957.

This PR adds the two missing fixture files to the Makefile.test.include.

@jonasschnelli jonasschnelli added the Tests label Jun 7, 2016

@jonasschnelli jonasschnelli changed the title from [Tests] fix missing test fixtures in Makefile.test.include to [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue Jun 7, 2016

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 7, 2016

Contributor

ACK 86efa30

Contributor

paveljanik commented Jun 7, 2016

ACK 86efa30

@paveljanik

This comment has been minimized.

Show comment
Hide comment
@paveljanik

paveljanik Jun 7, 2016

Contributor

Hmm, doesn't this call for generalization? Ie. to not need to name the files in this directory?

Contributor

paveljanik commented Jun 7, 2016

Hmm, doesn't this call for generalization? Ie. to not need to name the files in this directory?

@@ -206,7 +206,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const string& strInput)
// extract the optional sequence number
uint32_t nSequenceIn=std::numeric_limits<unsigned int>::max();
if (vStrInputParts.size() > 2)
nSequenceIn = atoi(vStrInputParts[2]);
nSequenceIn = std::stoul(vStrInputParts[2]);

This comment has been minimized.

@theuni

theuni Jun 7, 2016

Member

This should be stored to an unsigned long and checked against std::numeric_limits<uint32_t>::max() just to be safe, otherwise invalid values will wrap and be quietly accepted. To be doubly safe, it'd need to use strtoll and check errno and negatives :.

I'm unsure what amount of paranoia is called for here.

@theuni

theuni Jun 7, 2016

Member

This should be stored to an unsigned long and checked against std::numeric_limits<uint32_t>::max() just to be safe, otherwise invalid values will wrap and be quietly accepted. To be doubly safe, it'd need to use strtoll and check errno and negatives :.

I'm unsure what amount of paranoia is called for here.

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 7, 2016

Member

IMO we only support platforms/archs where int is always 32bit and I think std::numeric_limits<uint32_t>::max() won't change much.

@jonasschnelli

jonasschnelli Jun 7, 2016

Member

IMO we only support platforms/archs where int is always 32bit and I think std::numeric_limits<uint32_t>::max() won't change much.

This comment has been minimized.

@theuni

theuni Jun 7, 2016

Member

@jonasschnelli These are the cases I'm worried about (running on current master):

./bitcoin-tx 01000000000000000000 in=838af7eea675044a35a646f937ba26c412e4db51dc2b155c45f5c2675acd0cae:1:-1

0100000001ae0ccd5a67c2f5455c152bdc51dbe412c426ba37f946a6354a0475a6eef78a830100000000ffffffff0000000000
./bitcoin-tx 01000000000000000000 in=838af7eea675044a35a646f937ba26c412e4db51dc2b155c45f5c2675acd0cae:1:4294967296
0100000001ae0ccd5a67c2f5455c152bdc51dbe412c426ba37f946a6354a0475a6eef78a830100000000000000000000000000

In order to be able to test for those cases on the worst-case platform (win64), you need to store the input as a signed long long, since a signed long isn't big enough there.

@theuni

theuni Jun 7, 2016

Member

@jonasschnelli These are the cases I'm worried about (running on current master):

./bitcoin-tx 01000000000000000000 in=838af7eea675044a35a646f937ba26c412e4db51dc2b155c45f5c2675acd0cae:1:-1

0100000001ae0ccd5a67c2f5455c152bdc51dbe412c426ba37f946a6354a0475a6eef78a830100000000ffffffff0000000000
./bitcoin-tx 01000000000000000000 in=838af7eea675044a35a646f937ba26c412e4db51dc2b155c45f5c2675acd0cae:1:4294967296
0100000001ae0ccd5a67c2f5455c152bdc51dbe412c426ba37f946a6354a0475a6eef78a830100000000000000000000000000

In order to be able to test for those cases on the worst-case platform (win64), you need to store the input as a signed long long, since a signed long isn't big enough there.

This comment has been minimized.

@laanwj

laanwj Jun 8, 2016

Member

why not use ParseInt32, a function I especially wrote for this purpose and does the necessary checking?
Edit: as discused on IRC we really need a ParseUInt32, going to write one

@laanwj

laanwj Jun 8, 2016

Member

why not use ParseInt32, a function I especially wrote for this purpose and does the necessary checking?
Edit: as discused on IRC we really need a ParseUInt32, going to write one

This comment has been minimized.

@jonasschnelli

jonasschnelli Jun 8, 2016

Member

Right. ParseInt32 would be better. I guess it would require a ParseIntU32. This can be done in a later PR and also changes the rest of bitcoin-tx (there are servals atoi() and atoi64).

@jonasschnelli

jonasschnelli Jun 8, 2016

Member

Right. ParseInt32 would be better. I guess it would require a ParseIntU32. This can be done in a later PR and also changes the rest of bitcoin-tx (there are servals atoi() and atoi64).

@theuni

This comment has been minimized.

Show comment
Hide comment
@theuni

theuni Jun 8, 2016

Member

For the sake of unborking master, ACK 86efa30. I'll address my complaints in a follow-up.

Member

theuni commented Jun 8, 2016

For the sake of unborking master, ACK 86efa30. I'll address my complaints in a follow-up.

@laanwj laanwj merged commit 86efa30 into bitcoin:master Jun 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

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

Merge #8164: [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi i…
…ssue

86efa30 [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue (Jonas Schnelli)

laanwj added a commit to laanwj/bitcoin that referenced this pull request Jun 8, 2016

util: Add ParseUInt32 and ParseUInt64
Add error and range-checking parsers for unsigned 32 and 64 bit numbers.
The 32-bit variant is required for parsing sequence numbers from the
command line in `bitcoin-tx` (see #8164 for discussion). I've thrown in
the 64-bit variant as a bonus, as I'm sure it will be needed at some
point.

Also adds tests, and updates `developer-notes.md`.

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

Merge #8164: [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi i…
…ssue

86efa30 [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue (Jonas Schnelli)

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

Merge #8164: [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi i…
…ssue

86efa30 [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue (Jonas Schnelli)

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

Merge #8164: [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi i…
…ssue

86efa30 [Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue (Jonas Schnelli)

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