go-coap Feature/fix blockwise requires message

Summary by CodeRabbit

New Features
- Block-wise transfers now track exchanges per remote address for more reliable per-peer handling.
Refactor
- Block-wise APIs and internal flows updated to propagate remote address; TCP and UDP clients adjusted accordingly.
Tests
- Added UDP libcoap-based blockwise tests (GET/PUT/POST) with large payloads and test utilities.
Chores
- Examples switched to UDP port 5685.
- .gitignore adjusted to ignore only the repository root IDE directory (/.idea/).

Oct 08 '25 10:10 jkralik

Walkthrough

Block-wise transfer keying moved from token.Hash() to an exchangeKey (derived from token or remoteAddr). BlockWise APIs (Do, Handle) now accept a net.Addr and thread exchangeKey through cache/handlers. Client call-sites updated to pass RemoteAddr(). Misc: root .gitignore tightened, example UDP ports changed, libcoap tests and CI install steps added.

Changes

Cohort / File(s)	Summary
Git ignore scope `/.gitignore`	Changed ignore pattern from `.idea/` to `/.idea/`, restricting the ignore to the repository root `.idea` directory.
Example ports `examples/simple/client/main.go`, `examples/simple/server/main.go`	Updated example UDP port from `:5688` to `:5685` (client dial target and server ListenAndServe).
Blockwise exchangeKey integration & API changes `net/blockwise/blockwise.go`	Replaced token.Hash()-based cache keys with an exchangeKey derived via `getExchangeKey(token, addr)`; threaded `exchangeKey` (uint64) through internal helpers; updated `Do` and `Handle` signatures to accept `remoteAddr net.Addr`; added exchange-key-based register/get/cleanup flows and adjusted error paths.
Client call-site propagation (TCP) `tcp/client/conn.go`	Updated `blockWise.Do` and `blockWise.Handle` calls to pass `cc.RemoteAddr()` into blockwise processing.
Client call-site propagation (UDP) `udp/client/conn.go`	Updated `blockWise.Do` and `blockWise.Handle` calls to pass `cc.RemoteAddr()` into blockwise processing.
Blockwise tests (call adjustments) `net/blockwise/blockwise_test.go`	Adjusted test call sites to include the additional `nil`/addr parameter before callbacks; call mechanics changed but assertions/behavior unchanged.
New UDP libcoap tests `udp/server_libcoap_test.go`	Added libcoap-driven UDP blockwise tests (GET/PUT/POST), helper `sendCoAPRequestLibCoAP`, and `bigPayload10KiB` constant; tests require external `coap-client` binary.
CI: install coap-client `.github/workflows/test.yml`, `.github/workflows/test-for-fork.yml`	Added steps to install libcoap/coap-client on ubuntu/macOS runners to support the new libcoap tests.
DTLS/TLS test return consistency `dtls/server_test.go`, `tcp/server_test.go`	Adjusted helper functions to return consistent multi-value tuples on early error paths (propagate configs and error explicitly).
Router return consistency `mux/router.go`	`Router.Match` updated to explicitly return `(matchedRoute, matchedPattern)` in all control paths (including nil/no-match).

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor App
    participant ClientConn as ClientConn (TCP/UDP)
    participant BlockWise as BlockWise
    participant Keyer as getExchangeKey
    participant Cache as Exchange Cache

    App->>ClientConn: Do(req)
    ClientConn->>BlockWise: Do(req, maxSZX, maxMsgSize, RemoteAddr, doInternal)
    BlockWise->>Keyer: getExchangeKey(token, RemoteAddr)
    Keyer-->>BlockWise: exchangeKey / error
    alt exchangeKey OK
        BlockWise->>Cache: Lookup(exchangeKey)
        alt cached in-progress
            BlockWise-->>ClientConn: attach/wait
        else no cache
            BlockWise->>ClientConn: send blocks via doInternal
            ClientConn-->>BlockWise: responses
            BlockWise->>Cache: Update/Cleanup(exchangeKey)
        end
        BlockWise-->>App: final response
    else error
        BlockWise-->>App: error
    end

sequenceDiagram
    autonumber
    actor RemotePeer
    participant ServerConn as ServerConn
    participant BlockWise as BlockWise
    participant Keyer as getExchangeKey
    participant Cache as Exchange Cache
    participant Handler as next()

    RemotePeer->>ServerConn: Incoming message
    ServerConn->>BlockWise: Handle(w, r, maxSZX, maxMsgSize, RemoteAddr, next)
    BlockWise->>Keyer: getExchangeKey(r.Token, RemoteAddr)
    Keyer-->>BlockWise: exchangeKey / error
    alt exchangeKey OK
        BlockWise->>Cache: Get/Assemble(exchangeKey)
        BlockWise->>Handler: next(w, r)
        Handler-->>BlockWise: handled
        BlockWise->>Cache: Cleanup/Retain(exchangeKey)
    else error
        BlockWise-->>ServerConn: write error
    end

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pay special attention to net/blockwise/blockwise.go for correctness of exchangeKey derivation, concurrency around cache entries, and correctness of new/changed function signatures.
Verify all call-sites (TCP/UDP clients, tests) correctly pass remoteAddr and that tests compile/run with the new signatures.
Review new libcoap tests and CI workflow additions for platform assumptions and external binary availability.

Possibly related PRs

plgd-dev/go-coap#627 — Very similar refactor introducing exchangeKey and remoteAddr propagation in blockwise; likely duplicate/sibling change.
plgd-dev/go-coap#619 — Implements exchange-key based flow and Do/Handle signature changes; closely related.
plgd-dev/go-coap#566 — Related adjustments to DTLS/TLS test helper return signatures and error propagation.

Poem

I nibble bytes with whiskered grace,
I hash a token, then change my pace,
If tokens sleep, I ask the peer,
exchangeKey hops the cache right here.
Thump-thump — blocks hop home, a happy trace. 🐇✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 26.67% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title Check	❓ Inconclusive	The title "Feature/fix blockwise requires message" is vague and does not clearly convey the main purpose of the changeset. While it references "blockwise," which is indeed a central component of the changes, the phrase "requires message" is ambiguous and unclear. The title does not effectively communicate that the PR's primary objective is to refactor the blockwise implementation to accept a remote address parameter, derive an exchange key from either token hash or remote address for cache lookups, and enable RFC-compliant scenarios with empty tokens. A developer scanning git history would struggle to understand what this PR accomplishes without reading the full diff.	Consider revising the title to be more specific and descriptive. A clearer title might be: "Add remoteAddr parameter to blockwise Do/Handle for exchange-key-based caching" or "Support exchange-key-based cache lookups in blockwise for empty-token scenarios." The revised title should clearly indicate both what is being changed (blockwise method signatures) and why (to support RFC-compliant empty-token scenarios with remote-address-derived keys).

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

🧪 Generate unit tests (beta)

[ ] Create PR with unit tests
[ ] Post copyable unit tests in a comment
[ ] Commit unit tests in branch jkralik/fix/blockwise-requires-Message-ID

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 522b23e9c8c73f69d9c5b65c2dd22d71c9aea104 and 07c41fa0f291daccb451aee91cd12a1900e766d3.

📒 Files selected for processing (2)

.github/workflows/test-for-fork.yml (1 hunks)
.github/workflows/test.yml (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

.github/workflows/test.yml
.github/workflows/test-for-fork.yml

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)

GitHub Check: Fuzzing
GitHub Check: test (macOS-latest)
GitHub Check: test (ubuntu-latest)
GitHub Check: test1_20

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Oct 08 '25 10:10 coderabbitai[bot]

Codecov Report

:x: Patch coverage is 69.81132% with 32 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 73.27%. Comparing base (f73dcbd) to head (b66b2d0).

Files with missing lines	Patch %	Lines
net/blockwise/blockwise.go	70.58%	22 Missing and 8 partials :warning:
tcp/client/conn.go	0.00%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #626      +/-   ##
==========================================
+ Coverage   73.17%   73.27%   +0.10%     
==========================================
  Files          73       73              
  Lines        6986     7031      +45     
==========================================
+ Hits         5112     5152      +40     
- Misses       1489     1493       +4     
- Partials      385      386       +1

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

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

Oct 08 '25 12:10 codecov-commenter

Hi @jkralik, thanks for fixing the tests. Does anything else still needs to be done for it to be complete? Potentially a unit test with no tokens but remoteAddr set

Oct 09 '25 08:10 Theoyor

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
75.6% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Oct 09 '25 09:10 sonarqubecloud[bot]

Hi @jkralik, thanks for fixing the tests. Does anything else still needs to be done for it to be complete? Potentially a unit test with no tokens but remoteAddr set

Hi @Theoyor, Could you please create tests in UDP for the POST, PUT, GET, and Observe methods with the empty token case? Also, could you add some tests using the third-party library libCoAP, which is commonly used by IoT devices?

Oct 09 '25 09:10 jkralik

Sure, do you mean the compiled "coap-client" of libcoap? Is it already present in the testing environment?

Oct 09 '25 10:10 Theoyor

Sure, do you mean the compiled "coap-client" of libcoap? Is it already present in the testing environment?

No, we need to create it.

Oct 09 '25 10:10 jkralik

@jkralik I am not sure if I understand what you mean by "in UDP". Are there any tests for "with token" that I can use as reference to write mine and to know how to structure them as closely to the existing ones? Or should I do it in the blockwise_test.go file? Should I test for blockwise and non-blockwise?

Oct 09 '25 11:10 Theoyor

@jkralik I am not sure if I understand what you mean by "in UDP". Are there any tests for "with token" that I can use as reference to write mine and to know how to structure them as closely to the existing ones? Or should I do it in the blockwise_test.go file? Should I test for blockwise and non-blockwise?

I mean github.com/plgd-dev/go-coap/v3/udp package. eg https://github.com/plgd-dev/go-coap/blob/master/udp/client_test.go#L32

Oct 09 '25 12:10 jkralik

@jkralik I implemented a test using libcoap for GET PUT POST. DELETE is not used with a payload at all so I would leave it out. The tests were successful after a small fix.

I am not sure whether writing tests using cc.NewGetRequest(...) etc. would make any sense, as it doesn't support the option of no token (and doesn't need to). Still remaining is "Observe", which I will now start.

Do you think it is also necessary to include non-token tests into blockwise_test.go as well?

Oct 13 '25 15:10 Theoyor

@jkralik I implemented a test using libcoap for GET PUT POST. DELETE is not used with a payload at all so I would leave it out. The tests were successful after a small fix.

I am not sure whether writing tests using cc.NewGetRequest(...) etc. would make any sense, as it doesn't support the option of no token (and doesn't need to). Still remaining is "Observe", which I will now start.

Do you think it is also necessary to include non-token tests into blockwise_test.go as well?

It’s not necessary to include non-token tests in blockwise_test.go, as they are already covered through UDP.

Oct 14 '25 09:10 jkralik

So I found out that it seems coap observe does not work with blockwise as intended, neither token nor no-token. I will further dig into that

Oct 15 '25 12:10 Theoyor

@jkralik ok. I did some research and testing.

Let's start with what the standards say:

For Blockwise observe WITH token: https://datatracker.ietf.org/doc/html/rfc7959#section-3.4

the server first sends a direct response to the initial GET request (the resulting block-wise transfer is as in Figure 4 and has therefore been left out). The second transfer is started by a 2.05 notification that contains just the first block; the client then goes on to obtain the rest of the blocks starting from the second block.[...] Note [...]that the requests for additional blocks cannot make use of the token of the Observation relationship

For Blockwise observe WITHOUT token:

CoAP observe requires a token: https://datatracker.ietf.org/doc/html/rfc7641#section-1.2
if the coap server cannot or doesn't want to process the observation request may do so, and executes a normal GET response: https://datatracker.ietf.org/doc/html/rfc7641#section-4.1

What I found out during tests:

Not one tool from the three I tested can handle blockwise + observe correctly
- aiocoap doesn't implement it and fails with "notImplementedError"
- californiums cf-client doesn't send an observe and doesn't specify an option
- libcoap coap-client comes closest BUT uses the same token for the initial observe and follow up GET for the 2nd and onward block
go-coap cannot deal with obs + block WITHOUT token
When used with token the following two issues occur (tested with libcoap which keeps the token)
- The payload is transmitted by the server starting with first block after notification (should be the second)
- The observe is aborted after the first body is sent
- both issues might be due to the same token being used by libcoap but not sure.

My coclusion

Blockwise + observe doesn't work in either version of go-coap
The new change with exchangeKey doesn't change current behaviour in Blockwise + observe (I tested with coap-client using both master and my PR branch)
No major testing tool supports Blockwise + observe
I am not too deep into the go-coap code that I could change anything

My Proposal: I do a PR with the unit tests using coap-client onto thios branch here (so you can still modify before its merged onto master). Then we have blockwise without token working. Blcokwise + observe remains unchanged and untested. This should then be a different issue and PR.

What do you think?

Oct 15 '25 15:10 Theoyor

I created #627 for unit tests please take a look. It merges into this branch so you can make changes before it is merged to master

Also one would need to add coap-client into the test environment, which I cannot do.

Oct 15 '25 15:10 Theoyor

@jkralik any opinion?

Oct 22 '25 12:10 Theoyor

Hi @Theoyor . I have merged your changes and also I updated ubuntu/macos to install coap-client. But test with coap-client fails. Pls could you look at it ?

Oct 28 '25 13:10 jkralik

Hi @jkralik thanks for installing and merging. I was quite confused on why it doesn't work anymore. After some testing I found two issues. The first one is what we see more prominent in the error logs of the PR tests. However after fixing that locally, I encountered another, more complicated issue.

Long story short, I developed the testing using an older pre-compiled libcoap coap-client, and it worked. Now I self-compiled the coap-client for another project and the current coap-client sends a different token for each blockwise exchange for the same request. The issue is gone when I tested again with the old one.

Which version of coap-client did you install in the test environment? How should we procceed?

Oct 29 '25 15:10 Theoyor