Skip to content

Follow-up to BIP324 connection support #28433

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 11, 2023
Merged

Follow-up to BIP324 connection support #28433

merged 3 commits into from
Sep 11, 2023

Conversation

sipa
Copy link
Member

@sipa sipa commented Sep 8, 2023

This addresses a few remaining comments on #28196:

In addition, also fix an incorrect description in V2Transport::SendState (it claimed garbage was sent in the READY state, but it's in the AWAITING_KEY state).

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 8, 2023 •
edited
Loading

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

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theStack, naumenkogs
Stale ACK Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #28331 (BIP324 integration by sipa)
  • #24748 (test/BIP324: functional tests for v2 P2P encryption by stratospher)

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.

@sipa sipa mentioned this pull request Sep 8, 2023
Merged
@sipa sipa force-pushed the 202309_bip324_connection_followup branch from 8088b05 to 361ffb9 Compare September 8, 2023 19:41
@sipa sipa mentioned this pull request Sep 8, 2023
Closed
43 tasks
@sipa sipa force-pushed the 202309_bip324_connection_followup branch from 361ffb9 to ca758a7 Compare September 8, 2023 20:17
@Sjors
Copy link
Member

Sjors commented Sep 8, 2023

I'm not sure what you mean with this commit message:

This allows us to write the random-key V2Transport constructor in function of the explicit-key
one.

@sipa
Copy link
Member Author

sipa commented Sep 8, 2023 •
edited
Loading

I'm not sure what you mean with this commit message:

This allows us to write the random-key V2Transport constructor in function of the explicit-key one.

It lets us write V2Transport::V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in) noexcept using delegation to V2Transport(NodeId nodeid, bool initiating, int type_in, int version_in, const CKey& key, Span<const std::byte> ent32, std::vector<uint8_t> garbage) noexcept.

@sipa sipa force-pushed the 202309_bip324_connection_followup branch from ca758a7 to 2d46e5c Compare September 8, 2023 20:34
@DrahtBot DrahtBot mentioned this pull request Sep 8, 2023
Merged
Sjors
Sjors approved these changes Sep 9, 2023
View reviewed changes
Copy link
Member

@Sjors Sjors left a comment

Choose a reason for hiding this comment

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

utACK 2d46e5c

(I also cherry-picked this on the integration PR and running that now)

@sipa sipa force-pushed the 202309_bip324_connection_followup branch from 2d46e5c to b45a16b Compare September 9, 2023 14:57
@Sjors
Copy link
Member

Sjors commented Sep 9, 2023

re-utACK b45a16b

@sipa sipa force-pushed the 202309_bip324_connection_followup branch from b45a16b to 0f816b9 Compare September 10, 2023 01:45
@DrahtBot DrahtBot mentioned this pull request Sep 10, 2023
Merged
sipa added 3 commits September 10, 2023 16:11
@sipa
net: merge V2Transport constructors, move key gen
b6934fd 
This removes the ability for BIP324Cipher to generate its own key, moving that
responsibility to the caller (mostly, V2Transport). This allows us to write
the random-key V2Transport constructor by delegating to the explicit-key one.
@sipa
net: do not use send buffer to store/cache garbage
9bde93d 
Before this commit the V2Transport::m_send_buffer is used to store the
garbage:
* During MAYBE_V1 state, it's there despite not being sent.
* During AWAITING_KEY state, while it is being sent.
* At the end of the AWAITING_KEY state it cannot be wiped as it's still
  needed to compute the garbage authentication packet.

Change this by introducing a separate m_send_garbage field, taking over
the first and last role listed above. This means the garbage is only in
the send buffer when it's actually being sent, removing a few special
cases related to this.
@sipa
doc: fix typos and mistakes in BIP324 code comments
6470438
@sipa sipa force-pushed the 202309_bip324_connection_followup branch from 0f816b9 to 6470438 Compare September 10, 2023 20:12
theStack
theStack approved these changes Sep 10, 2023
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK 6470438

In terms of readability I think it's nice that there are now dedicated functions/methods for generating garbage and putting pubkey+garbage into the send buffer. The GenerateRandomKey helper might be useful in other places too (unit tests, wallet etc.), but that's out of scope for this PR; it can easily be moved to some shared moudule in e.g. src/util (or even directly key.h/key.cpp) later if needed.

@DrahtBot DrahtBot requested a review from Sjors September 10, 2023 22:07
@naumenkogs
Copy link
Member

ACK 6470438

@fanquake fanquake merged commit 8f7b9eb into bitcoin:master Sep 11, 2023
@theStack theStack mentioned this pull request Sep 12, 2023
Merged
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2023
@fanquake
Merge bitcoin#28433: Follow-up to BIP324 connection support
4bbfc7f 
6470438 doc: fix typos and mistakes in BIP324 code comments (Pieter Wuille)
9bde93d net: do not use send buffer to store/cache garbage (Pieter Wuille)
b6934fd net: merge V2Transport constructors, move key gen (Pieter Wuille)

Pull request description:

  This addresses a few remaining comments on bitcoin#28196:

  * Deduplicate the `V2Transport` constructors (bitcoin#28196 (comment))
  * Do not use the send buffer to store garbage (bitcoin#28196 (comment))
  * Fix typo (bitcoin#28196 (comment))

  In addition, also fix an incorrect description in `V2Transport::SendState` (it claimed garbage was sent in the `READY` state, but it's in the `AWAITING_KEY` state).

ACKs for top commit:
  naumenkogs:
    ACK 6470438
  theStack:
    Code-review ACK 6470438

Tree-SHA512: 4bf6d2fe73c8054502d0b60e9de1722f8b3dd269c2dd6bf67197c3fb6eabcf047b6360cdab3c1fd5504215c2ac4ac2890a022780efc30ff583776242c8112451
achow101 added a commit that referenced this pull request Jan 2, 2024
@achow101
Merge #28455: refactor: share and use GenerateRandomKey helper
2652506 
fa1d495 refactor: share and use `GenerateRandomKey` helper (Sebastian Falbesoner)

Pull request description:

  Making the `GeneratingRandomKey` helper (recently introduced in PR #28433, commit b6934fd) available to other modules via key.{h.cpp} allows us to create random private keys directly at CKey instantiation, in contrast to the currently needed two-step process of creating an (invalid) CKey instance first and then having to call `MakeNewKey(...)`.

  This is mostly used in unit tests and a few instances in the wallet.

ACKs for top commit:
  Sjors:
    re-ACK fa1d495
  achow101:
    ACK fa1d495
  sipa:
    utACK fa1d495
  kristapsk:
    cr utACK fa1d495
  stratospher:
    ACK fa1d495.

Tree-SHA512: 6fec73f33efe5bd77ca7d3c2fc06725d96f789f229294c39377e682ff222cfc7990b77c92e0bfd4cb6cf891d007ab1f86d395907511f06e87044bae37652a2fd
@bitcoin bitcoin locked and limited conversation to collaborators Sep 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@theStack theStack theStack approved these changes

@Sjors Sjors Awaiting requested review from Sjors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@sipa @DrahtBot @Sjors @naumenkogs @theStack @fanquake