Skip to content

Conversation

maflcko
Copy link
Member

@maflcko maflcko commented Aug 29, 2020

Adds an example where the heuristic fails

@DrahtBot DrahtBot added the Tests label Aug 29, 2020
@robot-dreams
Copy link
Contributor

Concept ACK

Would it make sense to be more explicit about what could go wrong (since it'll always be the case that the heuristic matches either iswitness=False or iswitness=True)?

For example:

    # The heuristic produces an absurd result with a negative output value
    heuristic = self.nodes[0].decoderawtransaction(hexstring=TX_HEURISTIC) 
    assert heuristic['vout'][0]['value'] < 0

    # The heuristic produces the same absurd result as iswitness=False
    non_witness = self.nodes[0].decoderawtransaction(hexstring=TX_HEURISTIC, iswitness=False)
    assert_equal(heuristic['txid'], non_witness['txid'])

    # Specifying iswitness=True produces a reasonable result
    witness = self.nodes[0].decoderawtransaction(hexstring=TX_HEURISTIC, iswitness=True)
    assert witness['vout'][0]['value'] > 0

@maflcko maflcko force-pushed the 2008-testDecodeRawT branch from fadb4ea to fa0a43c Compare October 13, 2020 12:18
@maflcko
Copy link
Member Author

maflcko commented Oct 13, 2020

`assert heuristic['vout'][0]['value'] < 0`

The amount doesn't have to be negative. I've modified the tx, so that the amount is positive.

@laanwj
Copy link
Member

laanwj commented Nov 23, 2020

I'm a bit confused by this. Is this something that should be able to fail, or a bug that needs to be corrected in the heuristic?

@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 9, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #22437 (test, refactor: add GetTransaction() coverage, improve rpc_rawtransaction by jonatack)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@adamjonas
Copy link
Member

@MarcoFalke mind addressing laanwj's question?

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 1, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?
  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@maflcko maflcko force-pushed the 2008-testDecodeRawT branch from fa0a43c to 57c0363 Compare December 24, 2021 12:37
@maflcko
Copy link
Member Author

maflcko commented Dec 24, 2021

Fixed by #20595

@maflcko maflcko closed this Dec 24, 2021
@maflcko maflcko deleted the 2008-testDecodeRawT branch December 24, 2021 12:46
@bitcoin bitcoin locked and limited conversation to collaborators Dec 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants