search

Sjors / bitcoin

Bitcoin Core integration/staging tree
https://bitcoin.org/en/download
MIT License
6 stars 10 forks source link

SockMan #64

Closed Sjors closed 3 days ago

vasild commented 3 days ago

Yo! There are two problems here:

  1. In Sv2Connman::EventReadyToSend() it forgot to erase the sent message from the queue, so it kept sending the same message over and over again. Fixed by adding client->m_send_messages.erase(). This fixes the failure of BOOST_REQUIRE_EQUAL(tester.LocalToRemoteBytes(), SV2_HEADER_ENCRYPTED_SIZE + 6 + Poly1305::TAGLEN);
  2. After this, at the exit it didn't properly interrupt SockMan. Fixed by adding interruptNet() in Sv2Connman::Interrupt().

Here is a patch: sv2sockman.diff.txt

The changes to sv2_connman_tests.cpp are not needed for the test to pass but I made them to unconfuse myself from the send vs recv interaction - since both ends of the connection are inside one program, I was confused what does it mean to "send" bytes. So I renamed a bunch of symbols to use the notation of "local" and "remote" and "local to remote" and "remote to local". Another possibility would be to use "us" and "them", e.g. "from us to them" and "from them to us". Feel free to ignore those.

Sjors commented 3 days ago

Thanks. I'll apply your changes as well as the test clarifications.

Sjors commented 3 days ago

Folded all of it into #50! Left a few followup questions there.

Additionally I fixed a few locking warnings and implemented EventGotEOF and EventGotPermanentReadError to just disconnect.