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-tests] Check return code #6827

Merged
merged 2 commits into from Oct 20, 2015

Conversation

Projects
None yet
6 participants
@MarcoFalke
Member

MarcoFalke commented Oct 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 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?

Member

jamesob commented Oct 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 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.

Member

MarcoFalke commented Oct 14, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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)

Contributor

TheBlueMatt commented Oct 14, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 14, 2015

Member

Agree with @TheBlueMatt

Member

jamesob commented Oct 14, 2015

Agree with @TheBlueMatt

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 14, 2015

Member

Done.

Member

MarcoFalke commented Oct 14, 2015

Done.

@jamesob

This comment has been minimized.

Show comment
Hide comment
@jamesob

jamesob Oct 14, 2015

Member

ACK

Member

jamesob commented Oct 14, 2015

ACK

@TheBlueMatt

View changes

Show outdated Hide outdated qa/rpc-tests/fundrawtransaction.py
# if the fee's positive delta is higher than this value tests will fail, neg. delta always fail the tests
# = 2 bytes * minRelayTxFeePerByte
feeTolerance = 2 * Decimal('0.00005000')/1000

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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

@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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

@TheBlueMatt

TheBlueMatt Oct 14, 2015

Contributor

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

This comment has been minimized.

@MarcoFalke

MarcoFalke Oct 15, 2015

Member

@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.

@MarcoFalke

MarcoFalke Oct 15, 2015

Member

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Oct 15, 2015

Member

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.

Member

jonasschnelli commented Oct 15, 2015

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

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 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.

Member

MarcoFalke commented Oct 15, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Oct 15, 2015

utACK

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Oct 19, 2015

Member

ACK

Member

morcos commented Oct 19, 2015

ACK

@TheBlueMatt

View changes

Show outdated Hide outdated qa/rpc-tests/fundrawtransaction.py
feeTolerance = Decimal(0.00000002) #if the fee's positive delta is higher than this value tests will fail, neg. delta always fail the tests
min_relay_tx_fee = self.nodes[0].getnetworkinfo()['relayfee']
# if the fee's positive delta is higher than this value tests will fail, neg. delta always fail the tests

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Oct 19, 2015

Contributor

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)

@TheBlueMatt

TheBlueMatt Oct 19, 2015

Contributor

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)

This comment has been minimized.

@MarcoFalke
@MarcoFalke

@laanwj laanwj added the Tests label Oct 20, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 20, 2015

Member

Looks good to me. This is the minimum to get the tests working again.

Member

laanwj commented Oct 20, 2015

Looks good to me. This is the minimum to get the tests working again.

@laanwj laanwj merged commit bd4c22e into bitcoin:master Oct 20, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Oct 20, 2015

Merge pull request #6827
bd4c22e [rpc-tests] Check return code (MarcoFalke)
0d8b175 [rpc-tests] fundrawtransaction: Update fee after minRelayTxFee increase (MarcoFalke)

@MarcoFalke MarcoFalke deleted the MarcoFalke:MarcoFalke-2015-rpcTestsReturnCode branch Oct 20, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment