i2p: clean up SESSION CREATE error logging #35117

1. 
   takeshikurosawaa commented at 8:06 PM on April 19, 2026: contributor

   Clean up the I2P SAM error path.

   SESSION CREATE may contain the private key, so the generic SAM reply error path now reports the redacted request text instead of the full request. It also avoids echoing raw router replies in those generic error messages.

   No network behavior change intended.

2. 
   DrahtBot commented at 8:07 PM on April 19, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35117.

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	davidgumberg, vasild
   Stale ACK	sedited

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

3. in src/test/i2p_tests.cpp:176 in c043f71291

    171 | @@ -172,4 +172,28 @@ BOOST_AUTO_TEST_CASE(damaged_private_key)
    172 |      }
    173 |  }
    174 |  
    175 | +
    176 | +BOOST_AUTO_TEST_CASE(session_create_reply_redacts_private_key)
   

   ---

   ---

   

   sedited commented at 8:44 AM on April 25, 2026:

   I don't think this test is a good way to enforce that we don't regress on printing the key. Can you drop it?

   ---

   

   sedited commented at 7:19 AM on April 26, 2026:

   Can you squash the commits?

4. luke-jr referenced this in commit 6f9467ff97 on May 3, 2026
5. sedited approved
6. 
   sedited commented at 3:47 PM on May 11, 2026: contributor

   ACK cd2833e7436d8546b2c948d984ea0be5dad7cb46

7. sedited requested review from vasild on May 11, 2026
8. 
   davidgumberg commented at 12:00 AM on May 12, 2026: contributor

   Please squash the commits.

9. i2p: clean up SAM error logging

   SESSION CREATE requests can contain the I2P private key. Keep using the redacted request text in error messages, and avoid echoing raw SAM replies in the generic reply error path.
   
   This keeps the error useful while avoiding logging either the private-key-bearing request or unescaped router-controlled reply bytes. No network behavior change is intended.

   b6c3670442
10. in src/i2p.cpp:323 in c043f71291

     319 | @@ -320,7 +320,7 @@ Session::Reply Session::SendRequestAndGetReply(const Sock& sock,
     320 |  
     321 |      if (check_result_ok && reply.Get("RESULT") != "OK") {
     322 |          throw std::runtime_error(
     323 | -            strprintf("Unexpected reply to \"%s\": \"%s\"", request, reply.full));
     324 | +            strprintf("Unexpected reply to \"%s\": \"%s\"", reply.request, reply.full));
    

    ---

    ---

    

    davidgumberg commented at 12:08 AM on May 12, 2026:

    Not an issue introduced in this PR but we should not be logging the raw bytes of the I2P router's reply, either this should be escaped somehow or we should avoid logging it:

                strprintf("Reply to \"%s\": had a RESULT not equal to OK.",  reply.request));
    
    

11. takeshikurosawaa force-pushed on May 15, 2026
12. 
    takeshikurosawaa commented at 2:00 PM on May 15, 2026: contributor

    Thanks @sedited and @davidgumberg.

    I squashed the branch into one commit and updated the error text so the non-OK SESSION CREATE path no longer logs the raw SAM reply. While touching the same helper, I also removed reply.full from the missing-key error message, so a malformed reply cannot fall back to logging the router-controlled payload before the non-OK check is reached.

    I kept the old unit-test removal as-is and did not add it back.

    Tested locally with:

    • cmake --build build_linux --target test_bitcoin
    • build_linux/bin/test_bitcoin --run_test=i2p_tests
    • ctest --test-dir build_linux --output-on-failure -R i2p_tests
13. 
    davidgumberg commented at 12:15 AM on May 16, 2026: contributor

    crACK https://github.com/bitcoin/bitcoin/pull/35117/commits/b6c367044288f83ab61c1a77ad34c33adf39ecf6

14. DrahtBot requested review from sedited on May 16, 2026
15. vasild approved
16. 
    vasild commented at 1:09 PM on May 18, 2026: contributor

    ACK b6c367044288f83ab61c1a77ad34c33adf39ecf6

17. fanquake merged this on May 18, 2026
18. fanquake closed this on May 18, 2026

    ---

Contributors

takeshikurosawaaDrahtBotsediteddavidgumbergvasild

Review Requested
sedited