Reuse server DHCPOFFER transaction id in DHCPREQUEST

[simonborje]

Hello! From my understanding of the DHCP specification rfc2131 smoltcp currently does not follow the specification when it comes to the transaction id used for DHCPREQUEST messages. I initially saw this mentioned in a discussion here esp-rs/esp-hal#2345 (reply in thread).

If I understand the spec. correctly, when a DHCPOFFER is received from a server, the follow-up DHCPREQUEST should reuse the transaction id from the DHCPOFFER message. Currently a new random transaction idis used for the request instead.

The relevant parts of the RFC is Table 5 and the text just after it (page 36-37).

I have only been able to test this on my home setup so far but it seems to work as expected there at least. I'm not sure how to add a proper test for this or if the change might interact with retries in some way that needs additional handling.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.33%. Comparing base (e2b75e3) to head (48d10eb).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1061      +/-   ##
==========================================
- Coverage   81.17%   80.33%   -0.84%     
==========================================
  Files          81       81              
  Lines       28955    24341    -4614     
==========================================
- Hits        23503    19554    -3949     
+ Misses       5452     4787     -665     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[simonborje]

Thanks for the review!
Is there anything I should do before this can be merged? I see that some checks are marked as failed.

[Dirbaio]

this field is redundant. We know the xid of DHCPOFFER is the same as the xid of DHCPDISCOVER, which is stored in the Socket.transaction_id field.

[simonborje]

This seems to work as well - I see that the DHCP Discover and Request use the same transaction id.

[Dirbaio]

looks good. I got a few more questions tho:

• Should the xid of the DHCPREQUEST in renew/rebind also match the original xid of the DHCPOFFER? The current code is generating them randomly. I've read through the RFC but I don't see an obvious answer.
• The code is always calling random_transaction_id but then overwriting it in the Requesting branch. Maybe it'd be better to only call random_transaction_id in the branches where it's actually needed to avoid unnecessary RNG calls.

[simonborje]

I can't find a clear answer regarding DHCPREQUEST xid:s at renew/rebind in the RFC. However, both Windows ipconfig /renew, and linux dhclient enp1s0 seem to use new xid:s in the DHCPREQUEST when renewing.

[Dirbaio]

thanks for checking. looks good to me then 🚀