rpc, test: addpeeraddress test coverage, code simplify/constness #22043

pull jonatack wants to merge 2 commits into bitcoin:master from jonatack:addpeeraddress-improvements changing 2 files +37 −16

jonatack commented at 12:09 PM on May 24, 2021: member
- Add functional test coverage for rpc addpeeraddress
- Simplify addpeeraddress and improve code constness
test: addpeeraddress functional test coverage 6b1926cf1e
rpc: simplify addpeeraddress and improve code constness b36e0cd1b9
fanquake added the label Tests on May 24, 2021
fanquake added the label RPC/REST/ZMQ on May 24, 2021
jonatack force-pushed on May 24, 2021
klementtan approved

klementtan commented at 4:42 PM on May 24, 2021: contributor

ACK b36e0cd

Tested with the following scenarios: Success: peer added

./src/bitcoin-cli -signet addpeeraddress 1.1.1.1 38333  
{
  "success": true
}

# Check if peer address added
./src/bitcoin-cli -signet getnodeaddresses 0    
[
...,
  {
    "time": 1621873900,
    "services": 9,
    "address": "1.1.1.1",
    "port": 38333,
    "network": "ipv4"
  },
...
]

Fail: Peer address already added

# Continue after previous scenario
./src/bitcoin-cli -signet addpeeraddress 1.1.1.1 38333
{
  "success": false
}

Fail: Empty Address

./src/bitcoin-cli -signet addpeeraddress "" 38333
{
  "success": false
}

DrahtBot commented at 10:03 PM on May 24, 2021: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #21514 (p2p: Ignore ports on I2P addresses by vasild)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

reemuru commented at 3:44 AM on May 25, 2021: none

Tested ACK. Run on signet, Fedora 34 and Ubuntu 20.x

[user@fedora bitcoin]$ ./src/bitcoin-cli -signet addpeeraddress x.xxx.xxx.x 38333
{
  "success": true
}
[user@fedora bitcoin]$ ./src/bitcoin-cli -signet getnodeaddresses 0
[
  {
   "time": 1621866922,
    "services": 1033,
    "address": "xxxx:xxxx:xxxx:x:xxxx:xxxx:xxxx:xxxxx",
    "port": 38333,
    "network": "ipv6"
  }
]
[user@fedora bitcoin]$ ./src/bitcoin-cli -signet addpeeraddress SAMEADDRESS 38333
{
  "success": false
}
[user@fedora bitcoin]$ ./src/bitcoin-cli -signet addpeeraddress "" 38333
{
  "success": false
}

</details>

[user@fedora bitcoin]$ ./test/functional/rpc_net.py 
2021-05-25T03:37:33.624000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_ulr3n_ye
2021-05-25T03:37:34.200000Z TestFramework (INFO): Test getconnectioncount
2021-05-25T03:37:34.201000Z TestFramework (INFO): Test getpeerinfo
2021-05-25T03:37:37.237000Z TestFramework (INFO): Test getnettotals
2021-05-25T03:37:37.360000Z TestFramework (INFO): Test getnetworkinfo
2021-05-25T03:37:37.511000Z TestFramework (INFO): Test getaddednodeinfo
2021-05-25T03:37:37.528000Z TestFramework (INFO): Test service flags
2021-05-25T03:37:37.697000Z TestFramework (INFO): Test getnodeaddresses
2021-05-25T03:37:43.703000Z TestFramework (INFO): Test addpeeraddress
2021-05-25T03:37:43.766000Z TestFramework (INFO): Stopping nodes
2021-05-25T03:37:43.920000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_ulr3n_ye on exit
2021-05-25T03:37:43.921000Z TestFramework (INFO): Tests successful

</details>

reemuru approved

MarcoFalke commented at 7:39 AM on May 25, 2021: member

review ACK b36e0cd1b9d361ac6f9777c09328a13e9ee923be 💭

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash 9dce392a47e73871b02708975647ca954f17d4677bc589be4b34303e8fca8153 -

</details>

in src/rpc/net.cpp:942 in b36e0cd1b9

 952 | -        obj.pushKV("success", false);
 953 | -        return obj;
 954 | +    bool success{false};
 955 | +
 956 | +    if (LookupHost(addr_string, net_addr, false)) {
 957 | +        CAddress address{CAddress({net_addr, port}, ServiceFlags(NODE_NETWORK | NODE_WITNESS))};

MarcoFalke commented at 7:42 AM on May 25, 2021:

could remove duplicate CAddress constructor

        CAddress address{{net_addr, port}, ServiceFlags{NODE_NETWORK | NODE_WITNESS}};

MarcoFalke merged this on May 25, 2021
MarcoFalke closed this on May 25, 2021
sidhujag referenced this in commit 665922fd90 on May 25, 2021
jonatack deleted the branch on Jun 6, 2021
fanquake referenced this in commit 1cc123f405 on Jun 7, 2021
sidhujag referenced this in commit 8739d50db8 on Jun 9, 2021
gwillen referenced this in commit 76bb76c18c on Jun 1, 2022
DrahtBot locked this on Aug 18, 2022

Contributors

Labels

Linked (view graph)

#22169 p2p, rpc, fuzz: various tiny follow-ups

rpc, test: addpeeraddress test coverage, code simplify/constness #22043

Conflicts