fix: allow onion addresses and external peers in messages command#791
Conversation
|
@pinheadmz I investigated the rpc_test failure and I believe it's a pre-existing issue unrelated to my changes. |
|
14/16 checks passing. The only remaining failure is plugin_test which is a pre-existing issue in test_base.py, while investigating it I discovered that inspect.getsource() can't handle functools.partial objects, causing a TypeError that masks the real timeout error. I've opened a separate PR #795 to fix that. This PR is ready for review. |
pinheadmz
force-pushed
the
fix/messages-onion-addresses
branch
from
e13039e to
4189232
Compare
|
rebased on main after merging #803 |
There was a problem hiding this comment.
code review 4189232
Thanks for taking this on! I left a few comments below but in general, please squash any fixes you make within the branch. To me it looks like this whole PR could be one single commit.
And then, I think the approach could be improved. I think messages() could be entirely left alone, and only in get_messages() we fall back to whatever the exact value of tank_b: str is if we fail to map that tank name to an IP address with a directory in message_capture.
I don't think namespace_b is None should be a trigger for the alternate behavior.
Also I wonder if we can add the port internally? Instead of the user needing to suffix ._18444 or whatever?
nkatha23
force-pushed
the
fix/messages-onion-addresses
branch
from
4189232 to
263592b
Compare
- Add is_external_peer() helper to detect onion addresses and non-tank peers - Skip namespace parsing and kubectl IP lookups for external peers - Match message capture directories directly by address for external peers - Fix dot-split validation that would reject onion addresses containing dots - Add check_messages() test to onion_test.py to verify the fix Fixes bitcoin-dev-project#761 fix: remove original dot-split validation that rejected onion addresses fix: move check_messages inside OnionTest class fix: resolve namespace before get_messages to fix tank-to-tank message lookup The previous implementation used namespace_b = None as a sentinel for both 'known tank with no explicit namespace' and 'external/onion peer', causing known tanks like tank-0001 to fall into the external peer path in get_messages. This set tank_b_ip = 'tank-0001' instead of doing the kubectl IP lookup, so no message capture directories ever matched. Fix: resolve namespace_b via get_default_namespace_or() for known tanks before calling get_messages, so only genuine external peers arrive with namespace_b = None. Fixes rpc_test verack assertion failure introduced in previous commit. fix: refactor onion/external peer support based on review feedback
nkatha23
force-pushed
the
fix/messages-onion-addresses
branch
from
263592b to
71ef99b
Compare
|
Hello @pinheadmz, I've addressed all review feedback: Removed is_external_peer() helper |
pinheadmz
left a comment
pinheadmz
left a comment
There was a problem hiding this comment.
ACK 71ef99b
Great work THANK YOU!!!
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 71ef99bac0534753863403ffb7b04419b9150341
-----BEGIN PGP SIGNATURE-----
iQJPBAEBCAA5FiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmnmNTkbFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMCwzAAoJEOfimEtiick6rxUQAK/343MatPEwzfxchrN5
Rj3YxEHcCSpA80bLBPBMIMw2yg73qTiYuUcyk0e/TEGu/dDfzcVr/yy9bWNKEFjP
FBTzhhngq+wiGsICbd90FS7j1A7YjVIXLlIvPgp3jS32NExlzeY97py5fpi3T7oJ
L1rt2ml7O2ZVp3MHZujQ2feTQ31D6g/y/XICZ0sTsIgArcLc/svReJr4xcBc9Vc5
vfLq6YQFfxMLkQG/d5++ol2qCtV5+kPkOoE2/SQbYMewiAcKknw1qq/0nQmD0uwJ
JGXeRz2mX5vkus9Iv4i6MMs7B4M41v18vTbUSQ5KNwaGEha1uH5gwKw+mG4eppKG
VKqAAuUKva9MULmeCxYcEWbnfLF3sBGrO39AtWyAsRjZy+mnOoxmkPsn8gW/RigE
JW87esW4//2nctomfkG+V3KHGbGNT18xH3IEW43HyMW003lkpBuV7jB/OvCRasgm
JgarrUFFANg0PAb2ml9TJ2+5/b9WOHvt2JIKfX2kMNp/wjE1k7a2Quski8s38uWz
RmGmkC9rxYRBYKYhLBaCqnt91BM/+1m7wmLndoyIpgpnbcsqa+PuBMolDr1q4Bep
8l3JeEEUX1i3JyN1JXQUJah2oC6Fv296szS+9QKNlsg0VH761DGWTLWEGxADvmI7
8KMT05Z6EjeJRRHDk197uuvV
=JUKa
-----END PGP SIGNATURE-----
pinheadmz's public key is on openpgp.org
2 participants
Fixes #761