Simpler setban and new ban manipulation commands (p2p, rpc/rest/zmq)

https://github.com/bitcoin/bitcoin/pulls/19825

Host: dhruv  -  PR author: dhruv

The PR branch HEAD was e650bba at the time of this review club meeting.

Notes

  • The BanMan class manages two related but distinct concepts:

    1. Banning: Manually configured by the user via the setban RPC method. No outbound/inbound connections will be established with a banned address or subnet and the address will not be gossiped. Banned addresses are persisted in banlist.dat on shutdown and reloaded on startup.

    2. Discouragement: Peers are assigned a misbehavior score for sending us messages that violate the protocol (e.g. a too large payload, or headers that don’t connect to our chain). If a peer accumulates a misbehavior score of 100, we disconnect and discourage that address. Inbound connections from discouraged addresses are allowed but preferred for eviction. Outbound connections are not made to discouraged addresses, and those addresses are not gossiped. Discouraged addresses are not persisted to disk on shutdown.

  • Neither banning nor discouragement fully protect against a determined attacker, since IP addresses are inexpensive.

  • If implemented without care, automatic banning would increase the risk of a network split.

  • The BanMan interface allows banning and unbanning via CNetAddr and CSubNet. This PR consolidates to only accept CSubNet (an IP address is a subnet of 1).

  • This PR also adds two new RPC commands:

    1. setban listbanned ip: lists all the ban entries that include ip

    2. setban removeall ip: removes all ban entries that include ip

Questions

  1. What are some use cases for banning specific IP addresses or subnets?

  2. Is automatic banning/disconnection an effective DoS countermeasure for misbehaving peers?

  3. How can automatic banning/disconnection be abused by an attacker?

  4. Would well-formed setban ip add/setban ip remove RPC calls ever throw an error? Should they?

  5. On master, what happens if you try to ban the same IP address twice? How does this affect the UX for ban scripts?

  6. Before this PR, if setban 192.168.1.1/24 add is followed by setban 192.168.1.1 add, an error is thrown. However you can setban 192.168.1.1/32 add. Why does this happen?

  7. Are there any downsides to allowing overlapping ban entries as this PR does?

  8. How can the user figure out which ban entries might be blocking outbound connections to a certain IP (before and after this PR)?

  9. How can the user completely unban an IP (before and after this PR)?

Meeting Log

  118:00 <dhruvm> #startmeeting
  218:00 <glozow> hai
  318:00 <emzy> hi
  418:00 <dhruvm> Hello everyone! Welcome to the PR Review Club for #19825: rpc: simpler setban and new ban manipulation commands.
  518:00 <_0x0ff> hey
  618:00 <enjoying> hi
  718:00 <norisgOG_> hi
  818:00 <dhruvm> Please say hi so everyone knows who is in attendance.
  918:00 <AnthonyRonning> hi
 1018:00 <theStack> hi
 1118:00 <felixweis> hey hey heyyy
 1218:00 <murtyjones> hi
 1318:00 <oxtr> hi guys
 1418:00 <thomasb06> hi
 1518:00 <dhruvm> Anyone here for the first time?
 1618:00 <jnewbery> hi
 1718:00 <amiti> hih
 1818:01 <_0x0ff> this will be my first
 1918:01 <dhruvm> Welcome _0x0ff !
 2018:01 <setpill> hi
 2118:01 <enjoying> welcome _0x0ff
 2218:01 <dhruvm> A reminder of meeting convention: When you have a question, don't ask to ask, just ask.
 2318:01 <anir> hey everyone
 2418:01 <michaelfolkson> hi
 2518:01 <jnewbery> welcome _0x0ff!
 2618:01 <setpill> sorta kinda first time
 2718:01 <schmidty> howdy
 2818:01 <dhruvm> welcome setpill !
 2918:01 <effexzi> Hi
 3018:01 <jnewbery> sorta welcome setpill!
 3118:01 <ecola> hi
 3218:01 <_0x0ff> thanks everyone, happy to join!
 3318:01 <oxtr> This is my first time dhruvm
 3418:02 <dhruvm> hi oxtr !
 3518:02 <jnewbery> welcome oxtr :)
 3618:02 <dhruvm> Did everyone get a chance to review the PR? How about a quick y/n from everyone
 3718:02 <glozow> y
 3818:02 <_0x0ff> y
 3918:02 <murtyjones> y (briefly)
 4018:02 <AnthonyRonning> y
 4118:02 <setpill> y
 4218:02 <felixweis> y
 4318:02 <ecola> n
 4418:02 <thomasb06> y
 4518:02 <anir> y
 4618:02 <theStack> n
 4718:02 <emzy> n
 4818:02 <enjoying> y
 4918:02 <norisgOG_> 1/2 y
 5018:02 <effexzi> N
 5118:02 <oxtr> n
 5218:02 <dhruvm> Awesome, let's get started.
 5318:02 <dhruvm> Q: What are some use cases for banning specific IP addresses or subnets?
 5418:03 <emzy> Ban a known IP address/network that is misbehaving or you don’t like. For example chain analyses companies.
 5518:03 <jonatack> hi
 5618:03 <felixweis> maybe you want to not allow connections from/to certain cloud providers. they have a lot of ip ranges
 5718:03 <thomasb06> it's rather for not up-to-date nodes or bugged nodes
 5818:03 <AnthonyRonning> Ban nodes not sending blocks or nodes attempting to swarm your node.
 5918:03 <ccdle12> hi
 6018:03 <anir> to keep the network healthy from bad actors such as spy nodes
 6118:03 <theStack> e.g. if nodes misbehave (i.e. violate the protocol) and discouragement them is not enough for us
 6218:03 <theStack> *discouraging
 6318:04 <ecola> avoid being eclipsed
 6418:04 <dhruvm> that's right emzy felixweis anir theStack !
 6518:04 <dhruvm> Those are all great answers! As an example, we may want to ban addresses or subnets that are used by crawlers like bitnodes.io, etc. There are people in the bitcoin community that maintain banlists shared widely.
 6618:05 <dhruvm> thomasb06: we use consensus rules and eviction logic for that
 6718:05 <thomasb06> ok
 6818:05 <dhruvm> AnthonyRonning: peers not sending blocks are rotated out using eviction logic
 6918:05 <dhruvm> (if necessary)
 7018:05 <AnthonyRonning> good to know!
 7118:05 <dhruvm> Q: Is automatic banning/disconnection an effective DoS countermeasure for misbehaving peers?
 7218:06 <jnewbery> ecola: banning IP addresses wouldn't protect you from being eclipsed
 7318:06 <murtyjones> To some extent, although IPs can be rotated.
 7418:06 <thomasb06> if the network is dense, it's efficient: a node can always find other nodes to gossip
 7518:06 <norisgOG_> dhruvm I guess its very cheap to change Ip addresses so I am a bit sceptic
 7618:06 <ccdle12> nope, it's pretty cheap for an attacker to use another IP address
 7718:06 <setpill> if there is a DoS vuln in bitcoin core you are not going to protect against it with selective bans,
 7818:06 <theStack> i think that question is also answered in the notes: "Neither banning nor discouragement fully protect against a determined attacker, since IP addresses are inexpensive."
 7918:06 <AnthonyRonning> maybe for low hanging fruit but not for persistent attackers due to IP's
 8018:06 <michaelfolkson> "we may want to ban addresses or subnets that are used by crawlers like bitnodes.io" Do people actually do this?
 8118:06 <setpill> it might work against a DDoS if the ip range is small
 8218:07 <felixweis> i was wondering, with asmap, could we also have ban behaviour where we ban certain ASes?
 8318:07 <anir> No because doing so risks splitting the network
 8418:07 <michaelfolkson> Monitor which addresses are on bitnodes and ban them?
 8518:07 <emzy> Yes. But doesn’t protect against an attacker fully.
 8618:07 <_0x0ff> can automatically banning (if flaswed logic) cause a split?
 8718:08 <dhruvm> murtyjones norisgOG_ ccdle12 setpill theStack you got it!
 8818:08 <dhruvm> Automatic banning/disconnection is not an effective DoS countermeasure as the primary key for banlists are IP addresses that are inexpensive. An attacker can change their banned identity for ~$0.01 per hour.
 8918:08 <glozow> _0x0ff: yes, if you were programmatically banning based on a rule that isn't consistent throughout the network
 9018:08 <dhruvm> michaelfolkson: i try pretty hard to ban bitnodes :)
 9118:08 <glozow> er by "you" i mean some part of the network
 9218:08 <enjoying> michaelfolkson I think dhruvm was saying, ban peers which are bitnodes, not ban nodes that are listed on bitnodes
 9318:08 <AnthonyRonning> what kind of information does bitnodes try to abuse?
 9418:09 <dhruvm> enjoying: yes
 9518:09 <felixweis> maybe give the rpc a single ip inside an AS we want to discourage connections to/from and then the system does a lookup into the ASMAP and deduces the rest
 9618:09 <dhruvm> AnthonyRonning: I don't know if they abuse it. Just seems strange. I'd rather they not know all the nodes.
 9718:10 <dhruvm> felixweis: today we only have ip and subnet bans. I know ASN bans have been discussed in the past.
 9818:10 <AnthonyRonning> are tor bans separate?
 9918:10 <enjoying> having dossiers on network topology isn't healthy for adversarial operating coniditions
10018:10 <felixweis> oh ok. thanks dhruvm
10118:10 <michaelfolkson> I would guess that would be really really hard. You need to not connect to nodes that might gossip your address to bitnodes
10218:10 <emzy> AnthonyRonning: no way to ban clients from tor seperatly.
10318:11 <AnthonyRonning> interesting
10418:11 <emzy> AnthonyRonning: that's by design
10518:11 <dhruvm> michaelfolkson: your addr might be gossiped but if they can't verify the node exists, they don't really know if the gossip is accurate
10618:11 <enjoying> emzy ban torrents *using* tor or ban peers connecting to you through tor ?
10718:12 <dhruvm> some of the conversation is leading nicely into the next question: How can automatic banning/disconnection be abused by an attacker, or create a network split?
10818:12 <enjoying> dhruvm I think verifying the node exists is less of a need, surveillance's goal is just to winnow the full set down to a more manageable set to go after
10918:13 <thomasb06> when a subnet is banned, it hides trustworthy nodes
11018:13 <enjoying> ban peers using tor*
11118:13 <AnthonyRonning> If there’s honest nodes on a subnet, a user could unintentionally ban the whole subnet an attack is using.
11218:13 <dhruvm> enjoying: wouldn't that leave surveillance open to a saturation attack with garbage addr calls?
11318:14 <setpill> if everyone programmatically bans a subnet that subnet is cut off
11418:14 <setpill> bans the same* subnet
11518:14 <_0x0ff> one misbehaving node could cause a ban of a subnet that belongs to other well behaving nodes
11618:14 <glozow> how would you get everyone to ban a specific subnet?
11718:14 <setpill> if it was included in bitcoin core
11818:15 <enjoying> dhruvm it would but would you agree, gossiped peer ip's that aren't actual nodes would be more surprising than them being actual nodes?
11918:15 <theStack> glozow: if it was automatic banning (there isn't, i guess because of the mentioned reasons :))
12018:16 <felixweis> dont think its possible right now, but if there would be logic that once you have many attacks coming from the same subnet but constantly changing ip suffixes it might feel like a mitigation
12118:16 <dhruvm> thomasb06 setpill _0x0ff: automatically banning a subnet for a misbehaving ip wouldn't really make sense. How would you select how large to make the subnet?
12218:16 <setpill> dhruvm: that wasn't the question ;)
12318:16 <dhruvm> enjoying: not really - it could be due to network settings, full slots, and many other reasons.
12418:16 <thomasb06> dhruvm: if several banned ip are in the same subnet
12518:17 <glozow> wait are we talking about a hardcoded automatic subnet ban, or are we talking about a programmatic rule for banning based on some misbehavior?
12618:17 <dhruvm> glozow: the latter
12718:17 <setpill> glozow: I was talking about the latter
12818:18 <setpill> Also talking about how it could *accidentally* lead to a netsplit rather than being abused by an attacker
12918:18 <glozow> o ok mhm
13018:18 <jnewbery> I think the question might be better phrased as "how *could* automatic banning..." We don't do automatic banning in Bitcoin Core. Why not?
13118:18 <setpill> I guess an attacker could read the hypothetical bitcoin core ban rules and create a situation that triggers them for a subnet
13218:19 <_0x0ff> exactly, it was just a hypotetical issue that could occur if it were implemented
13318:19 <emzy> enjoying: there is no from address in tor. So you can ban all clients or none.
13418:19 <setpill> (And this is why they aren't used)
13518:19 <dhruvm> setpill: you've got it. sorry for the question wording.
13618:19 <thomasb06> with an automatic banning, a node can become isolated
13718:19 <_0x0ff> automatic banning can also be very opinionated I guess
13818:19 <dhruvm> Automatic banning/disconnection can increase network partition risk if not implemented well. Segwit activation can illustrate this risk. Upgraded segwit nodes had stricter validation rules. If they punished older nodes that published invalid blocks(containing invalid segwit transactions), we would have partition risk. Luckily nodes are not punished in such cases after a recent consensus
13918:20 <dhruvm> change.
14018:20 <dhruvm> See: https://github.com/bitcoin/bitcoin/blob/7b975639ef93b50537a3ec6326b54d7218afc8da/src/net_processing.cpp#L1076
14118:20 <setpill> ahhh yeah segwit is a good case
14218:21 <jnewbery> dhruvm: this is a very important point. Everyone should try to understand it!
14318:21 â„ą SN27 is now known as S3CRE75
14418:21 <theStack> interesting. curious now to find out how exactly "recent" is defined
14518:21 <norisgOG_> dhruvm are you talking about an attack, if older node would construct invalid segwit tx?
14618:21 <jnewbery> If there's some rule that would cause Bitcoin Core to ban connections, then it could potentially be used to partition the network
14718:22 <setpill> norisgOG_: it would be an attack, a malicious entity could feed innocent pre-segwit nodes bad segwit txes that would lead to them being cut off by segwit nodes
14818:22 <dhruvm> norisgOG_: an older node, with less strict rules, could consider an invalid segwit tx input to be valid.
14918:22 <norisgOG_> setpill thanks
15018:23 <norisgOG_> dhruvm got it
15118:23 <norisgOG_> thx
15218:23 <glozow> fascinating, so scary
15318:23 <enjoying> emzy I think I'm not understanding something, are you saying that peers to connect through an onion address aren't specified in a manner we can selectively tell the node, "disconnect peer at jx2o6mp3dtql4bn2.onion" for example?
15418:23 <norisgOG_> you are talking about recent, does it change after some time
15518:23 <_0x0ff> i didn't consider the code in net_processing, i understand what you mean now
15618:23 <dhruvm> in that example, neither node is dishonest. we have to be careful in the definition of honesty.
15718:23 <jnewbery> The risk was more in a block containing an invalid segwit transaction. Old nodes would reject unconfirmed segwit transactions because of policy, but they'd accept blocks containing segwit transactions (which is what makes segwit a soft fork)
15818:24 <anir>  what happens if the network partitions?
15918:24 <setpill> dhruvm: but invalid segwit txes don't create themselves, so there would be a dishonest entity involved somewhere, right?
16018:24 <dhruvm> anir: hash rate is split and network security with it.
16118:24 <enjoying> anir decreased effective hashrate. possibility of double spends,
16218:24 <jnewbery> anir: there's no longer consensus of what the state of the ledger is
16318:24 <emzy> enjoying: yes there is no from address. Only hidden services have an to address.
16418:25 <dhruvm> setpill: maybe, but it's not the relaying node
16518:25 <michaelfolkson> There is a difference between chain split (half the network follows one chain, half the network another) due to one disputed SegWit transaction and peers banning other peers for sending a particular transaction
16618:25 <setpill> dhruvm: true
16718:25 <dhruvm> ok, let's keep rolling
16818:25 <dhruvm> Q: Would well-formed `setban ip add`/`setban ip remove` RPC calls ever throw an error? Should they?
16918:26 <glozow> u mean on master?
17018:26 <dhruvm> glozow: right, on master
17118:26 <_0x0ff> If user is adding/removing things in as it's expected it shouldn't. Errors should only be thrown when user is trying to add/remove something ambiguous
17218:26 <murtyjones> They could (and I think should) throw errors for malformed data being passed in
17318:26 <emzy> enjoying: they look like this: "127.0.0.1:57372"
17418:26 <dhruvm> _0x0ff: can you verify that with the code here? https://github.com/bitcoin/bitcoin/blob/22fa9673b02edf512d2a9fecf403955d225b97e5/src/rpc/net.cpp#L707
17518:27 <AnthonyRonning> In master, it does throw an error if the IP is already banned. I don't think it should because the intent is still correct even if already banned.
17618:27 <enjoying> emzy because bitcoind is proxying through the tor service on the box?
17718:27 <felixweis> it might stop scripts that feed a list into the rpc if the script exits on error
17818:27 <ccdle12> raised errors should be for something serious that justifies an exit
17918:27 <_0x0ff> ah, i didn't know we need to answer for what would happen with what's on master now. My answer was how it should be implemented.
18018:27 <emzy> enjoying: yes it looks like localhost 127.0.0.1
18118:28 <dhruvm> AnthonyRonning: ccdle12: exactly right!
18218:28 <dhruvm> On master, `setban ip add` throws an error if a subnet is already banned (as an ip or is a member of a banned subnet), and `setban ip remove` throws an error if we are trying to unban an ip/subnet that is not banned.
18318:28 <setpill> master throws JSONRPCError if the ip/subnet was already banned
18418:28 <dhruvm> #19825 takes the stance that no well-formed ban/unban instruction is illegal.
18518:28 <dhruvm> setpill: yup!
18618:28 <emzy> I get this: "error code: -23 / error message: / Error: IP/Subnet already banned"
18718:28 <felixweis> emzy: is it possible to make them look like they come from a different ip than 127.0.0.1? to differentiate them from local programs.
18818:29 <dhruvm> Q: On master, what happens if you try to ban the same IP address twice? How does this affect the UX for ban scripts?
18918:29 <dhruvm> (some of you have already answered the first part)
19018:29 <glozow> yeah it sounds inconvenient to error when there's not really anything wrong with the script
19118:29 <sishir> What about throwing an error after an updated as described by gmaxwell? https://github.com/bitcoin/bitcoin/pull/19825#issuecomment-683372477
19218:29 <_0x0ff> An error is thrown, which means ban scripts need to take that error into account and then remove the ban and re-add it. This complicates ban scripts because a narrower ban would be rejected in case there's a wider ban taken into effect and handling this is a mess.
19318:29 <glozow> oh hey sishir
19418:29 <emzy> felixweis: I think there is already someting like that in place. I'm not sure,
19518:30 <sishir> hello glozow
19618:30 <dhruvm> felixweis: glozow: _0x0ff: correct!
19718:30 <dhruvm> Banning an already banned address throws an rpc error: https://github.com/bitcoin/bitcoin/blob/7b975639ef93b50537a3ec6326b54d7218afc8da/src/rpc/net.cpp#L710
19818:30 <AnthonyRonning> Error is thrown in master, it makes it complicated for scripts to complete without error if they're pulling from a list and don't know exact state of the banlist.
19918:30 <dhruvm> this means that ban scripts are not idempotent
20018:31 <dhruvm> and scripts would exit on error making life harder
20118:31 <glozow> was there consideration for printing a warning or something? like "just fyi these were already banned"
20218:31 <glozow> just curious, not really suggesting that
20318:32 <dhruvm> glozow: it seems unnecessary as the user intent is already accomplished
20418:32 <dhruvm> a well-formed ban/unban instruction can't be illegal
20518:33 <dhruvm> Alright, next question.
20618:33 <dhruvm> Q: Before this PR, if setban 192.168.1.1/24 add is followed by setban 192.168.1.1 add, an error is thrown. However you can setban 192.168.1.1/32 add. Why does this happen?
20718:33 <emzy> This are separate lists. BanMan::Ban(CNetAddr&) and BanMan::Ban(CSubNet&)
20818:33 <AnthonyRonning> because /32 includes more IP’s in the subnet than /24 while 192.168.1.1 is already banned
20918:33 <setpill> because setban [..]/32 is a broader range, so it is not already banned by /24, so it isnt considered an error
21018:33 <_0x0ff> Because the /24 ban is wider and banman doesn't reject the narrower ban.
21118:33 <dhruvm> emzy: they are added to the same ban list, but via different functions
21218:34 <dhruvm> AnthonyRonning: /32 is 1 ip address, /24 is 256 addresses
21318:34 <AnthonyRonning> ohhh, got those mixed up
21418:34 <setpill> I made the same error lol
21518:35 <norisgOG_> dhruvm how often are these lists refreshed, in which thread do they run?
21618:35 <setpill> oh
21718:35 <dhruvm> Can you follow through to the `IsBanned` calls here: https://github.com/bitcoin/bitcoin/blob/22fa9673b02edf512d2a9fecf403955d225b97e5/src/rpc/net.cpp#L709 and investigate why this may be happening?
21818:35 <setpill> I get it
21918:35 <setpill> subnet bans are checked for *exact match* whereas ip bans are checked if they are contained in a subnet ban
22018:36 <dhruvm> norisgOG_: the lists are kept in memory and marked as dirty to persist to disk upon change
22118:36 <dhruvm> See `BanMan::DumpBanList`
22218:37 <dhruvm> setpill: that's correct!
22318:37 <dhruvm> On master, `BanMan::IsBanned(CNetAddr&)` checks if the provided ip address is a member of any banned subnet. However, `BanMan::IsBanned(CSubNet&)` only checks for exact matches: https://github.com/bitcoin/bitcoin/blob/master/src/banman.cpp#L77-L104. Since the rpc code for setban checks `IsBanned` prior to banning, the behavior is different for ip addresses and subnets.
22418:37 <AnthonyRonning> Is there any particular reason for that? Couldn't it deduce that there's no reason to add a smaller subnet if the larger is already banned?
22518:38 <dhruvm> emzy: both ip addresses and subnets are stored in the same ban list. they are both stored as subnets.
22618:38 <dhruvm> AnthonyRonning: just how the code happens to be structured on master. #19825 changes everything to be CSubNet calls.
22718:38 <setpill> AnthonyRonning: actually sishir just linked a comment by gmax that smaller subnet bans still make sense if they are longer duration than the bigger subnet ban
22818:39 <AnthonyRonning> yeah if they are longer, it makes sense.
22918:40 <dhruvm> AnthonyRonning: setpill: longer, narrower bans and shorter, wider bans make sense and there is potential to consolidate ban entries. However #19825 no longer consolidates.
23018:41 <dhruvm> So, that leads us to ask: Are there any downsides to allowing overlapping ban entries as this PR does?
23118:41 <AnthonyRonning> Possible internal overhead in tracking bans with minor differences, each requiring a lock and write to a file when added.
23218:41 <setpill> Very mild inefficiency
23318:41 <emzy> It will be hard to implement this logic into a program that uses the RPC.
23418:41 <setpill> what logic?
23518:42 <dhruvm> AnthonyRonning: setpill: yes!
23618:42 <dhruvm> emzy: that's right. any consolidation in ban entries can confuse scripts that are trying to maintain state
23718:42 <dhruvm> because their view of the ban list could look different than the output of `listbanned`
23818:43 <dhruvm> #19825 just adds new ban entries instead of trying to consolidate/reject overlapping ones. There is some cost to more entries in the banmap(memory) and increased cost of `IsBanned`(cpu/time). However, the most prominent place `IsBanned` is invoked is in `CConnman::AcceptConnection` which is relatively infrequent. The reduced code complexity is well-worth it.
23918:43 <ecola> idempotence is the key here
24018:43 <setpill> yeah, that's what I was thinking - less code complexity in bitcoin core is a worthy tradeoff for what seems like a small overhead
24118:44 <setpill> (hadn't considered the case of scripts that try to maintain a ban list)
24218:44 <dhruvm> setpill: that's what i have learnt as well
24318:44 <theStack> don't know what a typical banlist looks like but i could also imagine that overlapping ban ranges are actually quite rare, so it's even less of a problem?
24418:45 <dhruvm> Let's talk about the RPC use cases.
24518:45 <setpill> theStack: yeah and if not, write a better script :P
24618:45 <dhruvm> Q: How can the user figure out which ban entries might be blocking outbound connections to a certain IP (before and after this PR)?
24718:45 <_0x0ff> Before: user had to call `listbanned` which returned all banned ranged and look in there ... After: `listbanned <ip>` outputs all banned ranges that match a given ip
24818:45 <AnthonyRonning> `listbanned` command before, checking subnet; `listbanned ip` after.
24918:45 <theStack> setpill: true ;)
25018:46 <dhruvm> _0x0ff: AnthonyRonning: correct
25118:46 <emzy> before: “bitcoin-cli listbanned | less” and search
25218:46 <emzy> after: “listbanned x.x.x.x”
25318:46 <theStack> ah, that's a neat idea
25418:47 <dhruvm> How about unbanning an addr?
25518:47 <dhruvm> How can the user completely unban an IP (before and after this PR)?
25618:47 <_0x0ff> Before: i'm not 100% certain, but I believe user had to call `listbanned` and go over the list and manually call `setban <subnet> remove` in order to unban a particular IP ... After: `setban <ip> removeall` removes all subnets that would ban a given ip or subnet
25718:47 <AnthonyRonning> Before, `listbanned` and find all subnets to remove, or call `setban ip remove` on all subnets, ignoring errors if a subnet was not already banned. After, `setban removeall ip`
25818:48 <setpill> Before: 1. figure out all the rules that apply to the IP (using previously discussed methods) 2. call setban remove on all of them
25918:48 <setpill> After: setban removeall ip
26018:49 <setpill> Correction: setban ip removeall
26118:49 <dhruvm> _0x0ff: AnthonyRonning: setpill: you've got it! Hopefully it's easier to manage banlists if the PR is merged.
26218:49 <dhruvm> Well, those are all the quetions I have and we're a bit early, so are there any final questions anyone would like to ask?
26318:49 <_0x0ff> i like the change, makes the code a lot cleanear
26418:49 <sishir> I thought this feature was very cool and would love to test it out? Is there proper doc or any descriptions for testing?
26518:50 <dhruvm> thanks, _0x0ff
26618:50 <_0x0ff> what are some of the popular balists?
26718:50 <AnthonyRonning> agreed, idempotency is always great to have as an api consumer
26818:50 <setpill> dhruvm: why was IsBanned( const CSubNet& sub_net) removed?
26918:50 <ccdle12> sishir: there are updated tests in p2p_disconnect_ban.py
27018:50 <dhruvm> sishir: i'd recommend running bitcoind and using bitcoin-cli to emulate the functional tests in the code review
27118:50 <felixweis> i like the splitting up into different commits, was easier to follow the changes
27218:50 <theStack> i tried to understand the nodes are not punished in such cases after a recent consensus
27318:51 <dhruvm> setpill: it was not being used
27418:51 <sishir> ccdle12 dhruvm ooh okay! Thanks
27518:51 <dhruvm> felixweis: thank you for reviewing!
27618:51 <theStack> sorry -- * i tried to understand the "nodes are not punished in such ases after a recent consensus change" argument
27718:51 <michaelfolkson> Nice work updating the fuzz tests. Is that everything that is impacting the fuzz tests?
27818:51 <setpill> dhruvm: in a way this could be a regression for scripts that rely on the "error on existing ban rule" mechanism
27918:51 <theStack> i can't find any code that actually uses the BLOCK_RECENT_CONSENSUS_CHANGE enum. what am i missing?
28018:52 <setpill> if they tend to ban entire subnets, they now can't even check for them to avoid adding duplicates
28118:52 <theStack> s/uses/sets/
28218:52 <dhruvm> michaelfolkson: for this change, i think so
28318:52 <dhruvm> setpill: ah, i hadn't thought about that. but it shouldn't break the scripts as the intent will still be established.
28418:52 <AnthonyRonning> are there any procedures / process in place for changing the behavior of api's?
28518:52 <ccdle12> theStack: in MaybePunishNodeForBlock(), but the case statement now omits any logic on that
28618:52 <michaelfolkson> Why fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 11) -> fuzzed_data_provider.ConsumeIntegralInRange<int>(0, 6))?
28718:53 <setpill> hmm, well, i guess they could use setban listbanned for an ip and then check
28818:53 <setpill> the output
28918:53 <glozow> theStack: break in MaybePunish, i.e. don't punish
29018:53 <dhruvm> theStack: See the comment here: https://github.com/bitcoin/bitcoin/blob/4a540683ec40393d6369da1a9e02e45614db936d/src/consensus/validation.h#L28-L33
29118:53 <setpill> would it make sense to have setban listbanned subnet?
29218:53 <dhruvm> michaelfolkson: some of the integrals were not being used prior the change on master
29318:54 <dhruvm> michaelfolkson: and then some functions being tested were eliminated by #19825
29418:54 <theStack> glozow: ccdle12: dhruvm: yes, but in which part of the code a validation result was ever set to {TX,BLOCK}_RECENT_CONSENSUS_CHANGE?
29518:54 <glozow> theStack: I guess on master there aren't any recent consensus changes
29618:54 <michaelfolkson> Cool dhruvm, thanks
29718:54 <theStack> i guess it was in releases shortly after segwit, but i don't find any (maybe it has been renamed)
29818:55 <dhruvm> setpill: we decided to do removeall only for ips for now, but yes, i think it can make sense for subnets
29918:55 <dhruvm> theStack: we'd have to look in git history. jnewbery taught me yesterday that `git log -S` can be useful for such use cases
30018:56 <theStack> dhruvm: yes, that's what i used. maybe my local git history is not deep enough
30118:56 <jnewbery> `git log -S` is the BEST
30218:56 <dhruvm> git log -S BLOCK_RECENT_CONSENSUS_CHANGE
30318:56 <dhruvm> give me commit sha: a27a2957ed9afbe5a96caa5f0f4cbec730d27460
30418:57 <dhruvm> i guess that's a wrap. thank you everyone for coming to PR review club!
30518:57 <dhruvm> #endmeeting