[rpc-tests] Check return code #6827

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:MarcoFalke-2015-rpcTestsReturnCode changing 2 files +11 −3

MarcoFalke commented at 11:29 AM on October 14, 2015: member

This fixes #6826.

If this PR indeed fixes the issue, travis should fail and imo the PR should be merged nonetheless.

Update: The original commit was 027ca44 with the corresponding failing travis build. Due to requests I force pushed a rebase onto #6828.

(This closes #6828)
jamesob commented at 6:30 PM on October 14, 2015: member

Concept ACK

Edit: though I do think it awkward to merge something that's going to have expected build failures... should we assess how difficult it'd be to implement the fixes and attach them to this PR?
MarcoFalke commented at 6:36 PM on October 14, 2015: member

@jamesob I could only identify #6828. It should be safe to just merge #6828 before this one to prevent the red travis cross in the master branch.
TheBlueMatt commented at 6:43 PM on October 14, 2015: member

If that's the only one, can you just do them in the same PR? It seems overkill for each to have it's own.

On October 14, 2015 11:36:42 AM PDT, MarcoFalke notifications@github.com wrote:

@jamesob I could only identify #6828. It should be safe to just merge #6828 before this one to prevent the red travis cross in the master branch.

Reply to this email directly or view it on GitHub: #6827 (comment)
jamesob commented at 6:56 PM on October 14, 2015: member

Agree with @TheBlueMatt
MarcoFalke force-pushed on Oct 14, 2015
MarcoFalke commented at 7:39 PM on October 14, 2015: member

Done.
jamesob commented at 8:47 PM on October 14, 2015: member

ACK
in qa/rpc-tests/fundrawtransaction.py:None in 3855c7d43b outdated
```
  27 | @@ -28,7 +28,10 @@ def setup_network(self, split=False):
  28 |  
  29 |      def run_test(self):
  30 |          print "Mining blocks..."
  31 | -        feeTolerance = Decimal(0.00000002) #if the fee's positive delta is higher than this value tests will fail, neg. delta always fail the tests
  32 | +
  33 | +        # if the fee's positive delta is higher than this value tests will fail, neg. delta always fail the tests
  34 | +        #            = 2 bytes * minRelayTxFeePerByte
  35 | +        feeTolerance = 2 * Decimal('0.00005000')/1000
```
TheBlueMatt commented at 8:51 PM on October 14, 2015:

The right way is to fix the expected fees, not change the tolerance to nno longer catch errors.

TheBlueMatt commented at 8:52 PM on October 14, 2015:

The altnerative is just revert the minRelayTxFee stuff (ie #6722) and merge this afterwards.

MarcoFalke commented at 6:39 AM on October 15, 2015:

@jonasschnelli @TheBlueMatt Can you elaborate? I may be missing something but the rpc tests should not fail by changing the relayFee. Right now the bitcoin coin selection can only terminate if there is a tolerance of at least 2 bytes. (I.e. the transaction fee is paying for two bytes more than the tx actually has.) Consequently the feeTolerance is 2 bytes times whatever_you_pay_per_byte.

Regardless, #6828 (or this PR, which includes #6828) should be merged independent of #6722 because reverting minRelayTxFee is not "an alternative" to replacing this outdated magic number.
jonasschnelli commented at 6:37 AM on October 15, 2015: contributor

utACK. But agree with @TheBlueMatt. The fee tolerance change might be controversial. Not sure anymore, but when i first implemented this, fees had a tiny variance because of signature sizes, etc.

It would be nice if we could fix the fee tolerance in a proper way, although, very precise fees are not the main objective to test during fundrawtransaction rpc tests.
MarcoFalke force-pushed on Oct 15, 2015
MarcoFalke commented at 6:20 PM on October 15, 2015: member

@jonasschnelli I am not changing the behavior of fundrawtransaction. The fee tolerance was already there with 2 bytes * 1 sat/byte = 2 sat and due to #6793 I changed it to 2 bytes * 5 sat/byte = 10 sat. This should be uncontroversial and be enough to get the travis rpc tests back. (If you want to get rid of the 2 bytes tolerance, you'd have to adjust wallet code but that's just really out of scope of this PR)

Also, ping @laanwj for input.
jonasschnelli commented at 6:22 PM on October 15, 2015: contributor

utACK
morcos commented at 9:17 PM on October 19, 2015: member

ACK
[rpc-tests] fundrawtransaction: Update fee after minRelayTxFee increase 0d8b1759d2
[rpc-tests] Check return code bd4c22ed56

in qa/rpc-tests/fundrawtransaction.py:None in 6ce66fd255 outdated

  27 | @@ -28,7 +28,11 @@ def setup_network(self, split=False):
  28 |  
  29 |      def run_test(self):
  30 |          print "Mining blocks..."
  31 | -        feeTolerance = Decimal(0.00000002) #if the fee's positive delta is higher than this value tests will fail, neg. delta always fail the tests
  32 | +
  33 | +        min_relay_tx_fee = self.nodes[0].getnetworkinfo()['relayfee']
  34 | +        # if the fee's positive delta is higher than this value tests will fail, neg. delta always fail the tests

TheBlueMatt commented at 10:01 PM on October 19, 2015:

Can you update this comment to explain why this is picked (signatures may or may not take an extra byte or two, but never be smaller)

MarcoFalke commented at 10:00 AM on October 20, 2015:

@TheBlueMatt done.

MarcoFalke force-pushed on Oct 20, 2015
laanwj added the label Tests on Oct 20, 2015
laanwj commented at 10:20 AM on October 20, 2015: member

Looks good to me. This is the minimum to get the tests working again.
laanwj merged this on Oct 20, 2015
laanwj closed this on Oct 20, 2015
laanwj referenced this in commit 87e5539e9c on Oct 20, 2015
MarcoFalke deleted the branch on Oct 20, 2015
zkbot referenced this in commit 025bd44543 on Nov 21, 2020
zkbot referenced this in commit 7a0a268054 on Dec 2, 2020
zkbot referenced this in commit c8896f9907 on Dec 2, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#6826 [tests] rpc tests and travis never fail on asserts #6828 [rpc-tests] fundrawtransaction: Update fee after minRelayTxFee increase