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

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/04/rbf_base changing 2 files +4 −2

jonasschnelli commented at 6:01 PM on June 7, 2016: contributor

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 added the label Tests on Jun 7, 2016
[Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue 86efa30ae3
jonasschnelli force-pushed on Jun 7, 2016
jonasschnelli renamed this:
~~[Tests] fix missing test fixtures in Makefile.test.include~~
[Bitcoin-Tx] fix missing test fixtures, fix 32bit atoi issue
on Jun 7, 2016
paveljanik commented at 8:16 PM on June 7, 2016: contributor

ACK https://github.com/bitcoin/bitcoin/pull/8164/commits/86efa30ae3fd36aa77b19ff0f70bb89be9ec308e
paveljanik commented at 8:17 PM on June 7, 2016: contributor

Hmm, doesn't this call for generalization? Ie. to not need to name the files in this directory?
in src/bitcoin-tx.cpp:None in 86efa30ae3
```
 205 | @@ -206,7 +206,7 @@ static void MutateTxAddInput(CMutableTransaction& tx, const string& strInput)
 206 |      // extract the optional sequence number
 207 |      uint32_t nSequenceIn=std::numeric_limits<unsigned int>::max();
 208 |      if (vStrInputParts.size() > 2)
 209 | -        nSequenceIn = atoi(vStrInputParts[2]);
 210 | +        nSequenceIn = std::stoul(vStrInputParts[2]);
```
theuni commented at 8:46 PM on June 7, 2016:

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.

jonasschnelli commented at 8:51 PM on June 7, 2016:

IMO we only support platforms/archs where int is always 32bit and I think std::numeric_limits<uint32_t>::max() won't change much.
theuni commented at 9:29 PM on June 7, 2016:

@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.
laanwj commented at 7:33 AM on June 8, 2016:

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

jonasschnelli commented at 7:59 AM on June 8, 2016:

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 commented at 2:51 AM on June 8, 2016: member

For the sake of unborking master, ACK https://github.com/bitcoin/bitcoin/pull/8164/commits/86efa30ae3fd36aa77b19ff0f70bb89be9ec308e. I'll address my complaints in a follow-up.
laanwj merged this on Jun 8, 2016
laanwj closed this on Jun 8, 2016
laanwj referenced this in commit 0f24eaf253 on Jun 8, 2016
laanwj referenced this in commit 2df32a80bf on Jun 8, 2016
laanwj referenced this in commit 95642d2262 on Jun 8, 2016
laanwj referenced this in commit e012f3cea0 on Jun 8, 2016
codablock referenced this in commit 15861b3bb6 on Sep 16, 2017
codablock referenced this in commit 36efefa783 on Sep 19, 2017
codablock referenced this in commit 0b8169d498 on Dec 22, 2017
sickpig referenced this in commit 6c5d05a107 on May 4, 2018
sickpig referenced this in commit 638b66ecdb on May 9, 2018
lateminer referenced this in commit 853cd3845c on Oct 21, 2018
andvgal referenced this in commit 4e77f7637a on Jan 6, 2019
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#7957 [RPC][Bitcoin-TX] Add support for sequence number #8168 util: Add ParseUInt32 and ParseUInt64