Add m_bantime to Connman options #11434

benma commented at 11:42 AM on October 1, 2017: none

No description provided.

fanquake added the label P2P on Oct 1, 2017

fanquake added the label Refactoring on Oct 1, 2017

in src/init.cpp:1671 in 7028fb0b70 outdated

1665 | @@ -1666,7 +1666,10 @@ bool AppInitMain(boost::thread_group& threadGroup, CScheduler& scheduler)
1666 |      connOptions.nSendBufferMaxSize = 1000*gArgs.GetArg("-maxsendbuffer", DEFAULT_MAXSENDBUFFER);
1667 |      connOptions.nReceiveFloodSize = 1000*gArgs.GetArg("-maxreceivebuffer", DEFAULT_MAXRECEIVEBUFFER);
1668 |      connOptions.m_added_nodes = gArgs.GetArgs("-addnode");
1669 | -
1670 | +    connOptions.m_bantime = gArgs.GetArg("-bantime", DEFAULT_MISBEHAVING_BANTIME);
1671 | +    if (connOptions.m_bantime <= 0) {
1672 | +        return InitError(strprintf(_("%s cannot be configured with a negative value."), "-bantime"));

promag commented at 1:53 PM on October 1, 2017:

Add test?

benma commented at 7:25 AM on October 2, 2017:

Could you point me to where bitcoind/cli args are tested? Can't seem to find any example.

promag commented at 1:45 PM on October 2, 2017:

Search for assert_raises_process_error.

benma commented at 2:28 PM on October 2, 2017:

It is only used in TestBitcoinCli, which doesn't look like the right place. It looks like bitcoind command line args are not tested at all.

Shall we keep the check, but untested, like all the others?

promag commented at 2:44 PM on October 2, 2017:

No, it's not the right place. I was suggesting to add a right place :) IMO they should be tested but I may be alone here.

benma commented at 4:50 PM on October 2, 2017:

I don't disagree, but I think it's out of scope for this PR. Adding the test suite for this is needs to be prepared separately.

jnewbery commented at 6:56 PM on October 2, 2017:

You need assert_start_raises_init_error(). See https://github.com/bitcoin/bitcoin/blob/90926db2381d87c68858659873230a3811ebdce5/test/functional/multiwallet.py#L26 for example

promag commented at 7:03 PM on October 2, 2017:

That's right! Sorry for the misleading assert_raises_process_error.

in src/rpc/net.cpp:533 in 7028fb0b70 outdated

 529 | @@ -530,18 +530,22 @@ UniValue setban(const JSONRPCRequest& request)
 530 |  
 531 |      if (strCommand == "add")
 532 |      {
 533 | -        if (isSubnet ? g_connman->IsBanned(subNet) : g_connman->IsBanned(netAddr))
 534 | +        if (isSubnet ? g_connman->IsBanned(subNet) : g_connman->IsBanned(netAddr)) {

promag commented at 1:58 PM on October 1, 2017:

Unrelated. IMO remove change, otherwise fix others.

benma commented at 7:27 AM on October 2, 2017:

I put it into a separate commit so git blame doesn't show an unrelated commit description in those lines.

I kept them however. Formatting changes adhere to the dev guide and so far we've been fixing up parts as we work touch during the work.

promag commented at 1:59 PM on October 1, 2017: member

utACK. Care to explain the motivation?

formatting improvements related to rpc ban 3a4712c251

Add m_bantime to Connman options

More work towards a CConnman with no globals, so that we can test
instances against themselves.

d7d35b3d4c

benma force-pushed on Oct 2, 2017

benma commented at 2:28 PM on October 2, 2017: none

Pinging @theuni

theuni commented at 11:49 PM on October 2, 2017: member

mmm, this is another like #11387, but keeping it in CConnman is really not the right place for it :(

I'd really like to move the bandb out of CConnman. But for now, let's just force this value to be passed in by the caller rather than providing a default.

benma commented at 8:29 AM on October 3, 2017: none

The plan to move the ban db out of connman is a good one. I assume an instance of it would be passed to connman (or a callback or similar) so check for bans, which is easy to replace for mocking in tests.

Wherever the ban db code ends up, the PR that moves it is simpler after this PR. After all, the bandb class also shouldn't have globals and the bantime should be initialized in init.cpp.

If you don't mind, I'll take a stab at factoring out the ban code after (can coordinate on IRC beforehand).

But for now, let's just force this value to be passed in by the caller rather than providing a default.

What exactly do you mean? This line?

int64_t m_bantime = DEFAULT_MISBEHAVING_BANTIME;

theuni commented at 11:24 PM on October 5, 2017: member

The plan to move the ban db out of connman is a good one. I assume an instance of it would be passed to connman (or a callback or similar) so check for bans, which is easy to replace for mocking in tests.

Yep.

If you don't mind, I'll take a stab at factoring out the ban code after (can coordinate on IRC beforehand).

Sorry, I had already been working on that before I saw your response here :(. See #11457. I think we agree that's the cleaner fix for this?

If you'd be interested, it'd be great for CAddrMan to be moved out next, the exact same way. We should definitely discuss on IRC, since our goals seem to be aligned, and because I'm slowing you down more than helping :(. Feel free to ping me (cfields) there any time.

benma commented at 6:57 AM on October 6, 2017: none

The description of your PR alone won me over 😆 Closing this.

Thanks for the offer.

benma closed this on Oct 6, 2017

DrahtBot locked this on Sep 8, 2021