Skip to content
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

Update transaction malleability docs for RFCS #931

Merged
merged 2 commits into from Nov 18, 2020

Conversation

mDuo13
Copy link
Collaborator

@mDuo13 mDuo13 commented Nov 17, 2020

  • Update mentions of RequireFullyCanonicalSig to reflect that it's enabled now
  • De-emphasizes tfFullyCanonicalSig now that it has no effect
  • Rework the Transaction Malleability Exploit to reflect the multi-signing scenario, which is the only possible one now.

- Update mentions of RequireFullyCanonicalSig to reflect that it's
  enabled now
- De-emphasizes tfFullyCanonicalSig now that it has no effect
- Rework the Transaction Malleability Exploit to reflect the
  multi-signing scenario, which is the only possible one now.
@@ -66,11 +66,11 @@ The only flag that applies globally to all transactions is as follows:

| Flag Name | Hex Value | Decimal Value | Description |
|:----------------------|:-----------|:--------------|:--------------------------|
| `tfFullyCanonicalSig` | `0x80000000` | 2147483648 | _(Strongly recommended)_ Require a fully-canonical signature. |
| `tfFullyCanonicalSig` | `0x80000000` | 2147483648 | **DEPRECATED** No effect. Require a fully-canonical signature. |
Copy link
Contributor

Choose a reason for hiding this comment

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

@mDuo13 , should this be "requires"?

Copy link
Collaborator Author

@mDuo13 mDuo13 Nov 18, 2020

Choose a reason for hiding this comment

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

No, it's an imperative. As in, "The instructions to the server are, if this flag is enabled: require a fully canonical signature for this transaction." (But now, no effect, because that's required regardless. I should probably reword to make that last part more clear.)


2. The system notes the identifying hash of the vulnerable transaction, submits it to the XRP Ledger network, then begins monitoring for that hash to be included in a validated ledger version.

3. A malicious actor sees the transaction propagating through the network before it becomes confirmed.

4. The malicious actor calculates the alternate signature for the vulnerable transaction.
4. The malicious actor calculates removes an extra signature from the vulnerable transaction.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mDuo13 , just curious why this bullet is "The x actor" and the one above is "A x actor".

Copy link
Collaborator Author

@mDuo13 mDuo13 Nov 18, 2020

Choose a reason for hiding this comment

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

Step 3 is the first time a malicious actor is (definitely) part of the process, so it's "a" malicious actor. In step 4, I'm referring to the same malicious actor, so it's "the" malicious actor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Awkward phrasing: "calculates removes".

Makes more sense as "The malicious actor removes an extra signature..."


5. The malicious actor submits the modified (likely non-fully-canonical) transaction to the network.
5. The malicious actor submits the modified transaction to the network.
Copy link
Contributor

Choose a reason for hiding this comment

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

@mDuo13 , another "The x actor".

Copy link
Collaborator Author

@mDuo13 mDuo13 Nov 18, 2020

Choose a reason for hiding this comment

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

In this case, too, same malicious actor, thus, "the"

Copy link
Contributor

@gbarr01 gbarr01 left a comment

Choose a reason for hiding this comment

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

LGTM, just had a few small questions.

@mDuo13 mDuo13 merged commit b2716c6 into XRPLF:master Nov 18, 2020
@mDuo13 mDuo13 deleted the rfcs_enabled branch November 18, 2020 23:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants