Skip to content

Conversation

JeremyRubin
Copy link
Contributor

This allows an optional tx_invalid.json parameter at position 3 with the format:

[{"if_unset": ["A", "D"], "then_unset": ["B", "C"]}]

During the one-by-one flag mutation (patched in #22948 to be all combinations, hence the support of multiple) which checks that if disable required flags ["A", "D"] are all unset then the flags ["B", "C"] should become unset as well. The unsetting logic loops until the flags are unset (so rules like if_unset A then_unset B and if_unset B then_unset D end up with B and D unset if A is unset and B is set initially).

The current use case of this modification is:

[{"if_unset": ["DEFAULT_CHECK_TEMPLATE_VERIFY_HASH"], "then_unset": ["DISCOURAGE_UPGRADABLE_NOPS"]}]

This allows us to write use cases wherein we expect DEFAULT_CHECK_TEMPLATE_VERIFY_HASH to be enabled for the tx to be invalid, but desire for it to be valid (and not discouraged) when it is off. After activation of such a soft fork, we can remove such a rule.

We do not want to do this via a TrimFlags rule as it should not happen across all transactions, only the ones we specify.

It would also be possible to have a list of allowed unsettable flags (e.g., just DISCOURAGE_UPGRADABLE_NOPS and other standardness rules), but it is not required. While the allowing of multiple rules is perhaps extra, this keeps the format flexible for future needs.

@fanquake fanquake added the Tests label Sep 11, 2021
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 12, 2021

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

Conflicts

No conflicts as of last run.

@JeremyRubin
Copy link
Contributor Author

@MarcoFalke is their an automated tool to run to fix this PR?

@maflcko
Copy link
Member

maflcko commented Dec 13, 2021

@JeremyRubin I think git will always consider context, but you can use patch for an automated approach to ignore changes in adjacent lines:

$ git show -U0 7f4228566eafd91c3201e794ef2d82ca884db8c6 | patch -p1 --dry-run  
checking file src/test/data/tx_invalid.json
checking file src/test/transaction_tests.cpp
Hunk #1 succeeded at 289 (offset 1 line).
Hunk #2 succeeded at 297 (offset 1 line).
Hunk #3 succeeded at 391 (offset 1 line).
Hunk #4 succeeded at 394 (offset 1 line).
Hunk #5 succeeded at 411 (offset 1 line).

@JeremyRubin
Copy link
Contributor Author

i was asking because i think the conflict arose out of a scripted diff PR of yours that was merged, I was curious if you reckon that it will fix these conflicts too...

@JeremyRubin
Copy link
Contributor Author

failure is unrelated; has been rebased

Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

Concept -0 as this seems to only be useful for #21702, and not otherwise beneficial.

@bitcoin bitcoin locked and limited conversation to collaborators Dec 16, 2023
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