Skip to content

Conversation

@instagibbs
Copy link
Member

@instagibbs instagibbs commented Aug 16, 2016

  1. The tx entry from mapAlreadyAskedFor is erased as soon as it is received, and it doesn't enter the reject filter even though it's invalid, so we can immediately announce the transaction again and receive a getdata for that tx.
  2. An example transaction is being rejected due to premature witness, not size

@instagibbs instagibbs changed the title Update p2p-segwit.py to reflect correct AskFor behavior Update p2p-segwit.py to reflect correct behavior Aug 16, 2016
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this code could be commented better I think. I did intend for the transaction to be too large for standardness rules, to ensure that we were checking for premature witness before doing the IsStandard check (that change was a recent bugfix). Perhaps just improve the comment, rather than change the test itself?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly clear what you were shooting for, so feel free to suggest new text?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of this:

# We'll add an unnecessary witness to this transaction that would cause 
# it to be non-standard, to test that violating policy with a witness before 
# segwit activation doesn't blind a node to a transaction.  Transactions
# rejected for having a witness before segwit activation shouldn't be added
# to the rejection cache.

And then at line 312, add a comment:

# Note that this should be rejected for the premature witness reason,
# rather than a policy check, since segwit hasn't activated yet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, pushed

@sdaftuar
Copy link
Member

sdaftuar commented Sep 9, 2016

utACK

@btcdrak
Copy link
Contributor

btcdrak commented Sep 12, 2016

utACK 5547aeb

1 similar comment
@laanwj
Copy link
Member

laanwj commented Sep 13, 2016

utACK 5547aeb

@laanwj laanwj merged commit 5547aeb into bitcoin:master Sep 13, 2016
laanwj added a commit that referenced this pull request Sep 13, 2016
5547aeb p2psegwit.py transaction is rejected due to premature witness not size (instagibbs)
bc1d1f2 Update p2p-segwit.py to reflect correct AskFor behavior (instagibbs)
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Sep 21, 2016
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Sep 21, 2016
@laanwj
Copy link
Member

laanwj commented Sep 26, 2016

This is backported in #8772, removing needs-backport tag.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants