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

Refactor message transport packaging #16562

Merged
merged 1 commit into from
Feb 28, 2020

Conversation

jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Aug 7, 2019

This PR factors out transport packaging logic from CConnman::PushMessage().
It's similar to #16202 (where we refactor deserialization).

This allows implementing a new message transport protocol like BIP324.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 7, 2019

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

Conflicts

No conflicts as of last run.

Copy link
Member

@promag promag left a comment

Choose a reason for hiding this comment

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

This is more a concept comment than code review. Looks like this approach causes reallocations? If the goal is to factor out from CConnman::PushMessage have you considered something like class MessageWriterV1 : public MessageWriter? The message writer V1 would still push 2 buffers to vSendMsg (vSendMsg should be accessible to the message writer). In other words, CConnman wouldn't be responsible to push to vSendMsg).

src/net.h Outdated Show resolved Hide resolved
@jonasschnelli
Copy link
Contributor Author

@promag: yes. You'r right. The insert/reallocation was stupid. Slighly changed the approach. It is now also avoiding the behavior change.

Also removed the unused return value (will eventually come back with the v2 implementation).

Copy link
Member

@ariard ariard left a comment

Choose a reason for hiding this comment

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

ACK b840e42, code reviewed, tests ok. Feel free to add comments if you rebase.

src/net.h Show resolved Hide resolved
src/net.h Show resolved Hide resolved
src/net.h Outdated Show resolved Hide resolved
@dongcarl
Copy link
Contributor

Rebased this PR for you as well: master...dongcarl:2020-01-net_refactor_2-rebased

@jonasschnelli
Copy link
Contributor Author

Thanks @dongcarl. Pushed your rebased version.

Copy link
Contributor

@elichai elichai left a comment

Choose a reason for hiding this comment

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

semiACK 16d6113 ran functional+unit tests.

While reading the code I found one observable difference which is the replacement of HEADER_SIZE with serializedHeader.size(). I'm not exactly sure which should be used here(they should be the same but maybe there's some DOS vector i'm missing)
other than that the pointer is allocated in the constructor so there shouldn't be a way to call it when null.

@dongcarl
Copy link
Contributor

dongcarl commented Feb 4, 2020

ACK 16d6113 FWIW


Possible followup:

Add properly formatted Doxygen comments (something like what's already in the abstract class) for, and/or rename TransportSerializer::prepareForTransport as the name can be misinterpreted as: "do everything that we need to do before we send it to transport layer."

@ariard
Copy link
Member

ariard commented Feb 25, 2020

Code review ACK 16d6113

I'm not exactly sure which should be used here(they should be the same but maybe there's some DOS vector i'm missing)

@elichai I don't think there is a risk of DoS here, serializerHeader as all CMessageHeader components have a fixed-length. Bandwidth-DoS is already left unbounded (see default of -maxuploadtarget)

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

ACK 16d6113 🙎

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK 16d6113f4faa901e248adb693d4768a9e5019a16 🙎
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUhcGwv+Igl6tmkV78gSSi8EZt3582CuubMrSdBrB8UJ62ukEZVAmotmNR3ShTjM
rAmpiY7bxU0/2hW6Nr1YFo7t3FZ15dNPX2fTClctXXeLVFJdc1fSUwUg/oeRoJtm
T2CftFTfDrbU2o7HeX7rZvYSuieIl/SFBmNQ+w4jG8AFh4bCHH3pQVC7ueJhIHIx
qxKx9GNzwGOn3/rKCXj9vkEKRxHs/vGFDh4WYRezhEWkVvpe6996IiUMYVsIgyqm
npuGyOYoBmaSxj4z9PFHrL58AnnuRlUKy/1B0MyHz5v789b2swFn1iw5L9JTzOvh
dI9W2w7vkUnIaudUn1Z5dNPmVG8k7tmMAYvxE7TrhD1UgBRe2QYyNMt8CEJerxOd
50xlIKikLZxzL1ASwxatzPSKMl2ITprvIMrtoI0RrS1tPtfcXLnx10yr7EJ0dAxB
ZLs3GWZah5d/XLMDcyBSGWtiuWS+nkws4dAmZIlwBI/Qcm55dXAuW+Wocj59mtDE
QdnFfvp/
=gFtP
-----END PGP SIGNATURE-----

Timestamp of file with hash 2ae5b6b8bcff0b2640a243d5665c57ba947e1d6ef581e8eb4aa04cfd1b160d31 -

@@ -2703,6 +2716,7 @@ CNode::CNode(NodeId idIn, ServiceFlags nLocalServicesIn, int nMyStartingHeightIn
}

m_deserializer = MakeUnique<V1TransportDeserializer>(V1TransportDeserializer(Params().MessageStart(), SER_NETWORK, INIT_PROTO_VERSION));
m_serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());
Copy link
Member

Choose a reason for hiding this comment

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

Any reason this needs to call the copy(or move) constructor?

Suggested change
m_serializer = MakeUnique<V1TransportSerializer>(V1TransportSerializer());
m_serializer = MakeUnique<V1TransportSerializer>();

@practicalswift
Copy link
Contributor

practicalswift commented Feb 27, 2020

Any chance we can move forward with the V1TransportSerializer fuzzer in #17771 to get additional robustness testing of this code before proceeding with the V1TransportSerializer refactoring(s)?

It should hopefully be trivial to review :)

@maflcko maflcko merged commit eca4d8e into bitcoin:master Feb 28, 2020
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 29, 2020
16d6113 Refactor message transport packaging (Jonas Schnelli)

Pull request description:

  This PR factors out transport packaging logic from `CConnman::PushMessage()`.
  It's similar to bitcoin#16202 (where we refactor deserialization).

  This allows implementing a new message transport protocol like BIP324.

ACKs for top commit:
  dongcarl:
    ACK 16d6113 FWIW
  ariard:
    Code review ACK 16d6113
  elichai:
    semiACK 16d6113 ran functional+unit tests.
  MarcoFalke:
    ACK 16d6113 🙎

Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1
@jonasschnelli
Copy link
Contributor Author

@practicalswift: are you working on fuzz testing the serializing part?

sidhujag pushed a commit to syscoin-core/syscoin that referenced this pull request Nov 10, 2020
16d6113 Refactor message transport packaging (Jonas Schnelli)

Pull request description:

  This PR factors out transport packaging logic from `CConnman::PushMessage()`.
  It's similar to bitcoin#16202 (where we refactor deserialization).

  This allows implementing a new message transport protocol like BIP324.

ACKs for top commit:
  dongcarl:
    ACK 16d6113 FWIW
  ariard:
    Code review ACK 16d6113
  elichai:
    semiACK 16d6113 ran functional+unit tests.
  MarcoFalke:
    ACK 16d6113 🙎

Tree-SHA512: 8c2f8ab9f52e9b94327973ae15019a08109d5d9f9247492703a842827c5b5d634fc0411759e0bb316d824c586614b0220c2006410851933613bc143e58f7e6c1
jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Nov 13, 2020
Summary:
```
This PR factors out transport packaging logic from
CConnman::PushMessage().
It's similar to #16202 (where we refactor deserialization).

This allows implementing a new message transport protocol like BIP324.
```

Backport of core [[bitcoin/bitcoin#16562 | PR16562]].

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, PiRK, jasonbcox

Reviewed By: #bitcoin_abc, PiRK, jasonbcox

Differential Revision: https://reviews.bitcoinabc.org/D8374
@dhruv
Copy link
Contributor

dhruv commented May 24, 2021

@practicalswift: are you working on fuzz testing the serializing part?

@jonasschnelli: I've attempt to extend the test to serialization in #22029. I've also discovered that the deserialization does not proceed 60-70% of the time due to a failed checksum test. Will try to address that and increase coverage in another commit this week.

kwvg added a commit to kwvg/dash that referenced this pull request Nov 1, 2021
kwvg added a commit to kwvg/dash that referenced this pull request Nov 3, 2021
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 17, 2022
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

None yet

10 participants