rpc: Add admin_addTrustedPeer and admin_removeTrustedPeer. #16333
Conversation
shazow
force-pushed
the
addremovetrustedpeer
branch
from
d4fdc77
to
fda995c
Compare
|
I think the test failures are flakes. Anyone have a chance to take a look? |
shazow
force-pushed
the
addremovetrustedpeer
branch
from
fda995c
to
c1670af
Compare
|
Rebased on latest master. Polite ping for feedback please. :) |
shazow
force-pushed
the
addremovetrustedpeer
branch
from
c1670af
to
57b6379
Compare
|
Apologies for the long lead-time. It tentatively looks good to me, but could either @fjl or @zsfelfoldi PTAL at this one? |
|
Context for this PR: https://medium.com/@shazow/an-economic-incentive-for-running-ethereum-full-nodes-ecc0c9ebe22 (I like it!) |
|
@holiman Thanks! FWIW I've been using this patch in my live vipnode deployment for the last couple of weeks and it's working as expected. |
|
Any chance this could get tagged with the 1.8.9 milestone? I'd love to see this in the next release. |
|
Please @zsfelfoldi take a look? |
p2p/server.go
Outdated
| trusted[n.ID] = true | ||
| // Mark any already-connected peer as trusted | ||
| if p, ok := peers[n.ID]; ok { | ||
| p.rw.flags |= trustedConn |
There was a problem hiding this comment.
Since this can happen any time during the peer's lifecycle, can't this write collide with a read in func (p *Peer) Info() ? I am not very familiar with this part but it looks like we assumed until now that flags don't change after the peer is connected. @fjl opinion? Should we not use locks or atomic read/write if we change the flags of live peers?
There was a problem hiding this comment.
That's a good question. go test -race ./p2p didn't catch it but you might be right.
I'm happy to bolt the flags down with a mutex or atomic. Let me know what you'd prefer. :)
There was a problem hiding this comment.
I think it's OK to just add the ID to the trusted set without modifying flags of existing connections. The flag is not used after handshake anyway.
There was a problem hiding this comment.
@fjl Hm, I worry that has potential for breaking other code that relies on the peer state. Such as if you do admin.peers, it will return incorrect metadata, right?
There was a problem hiding this comment.
We would need a lock then and I want to avoid that ;)
There was a problem hiding this comment.
@fjl Just rebased and pushed another commit with more testing for this, still passes with -race on. I'm fairly convinced that these variables are only ever written and read inside the server event loop, so I don't think there is a race condition.
If you still prefer, I can get rid of updating the flags. We might be able to get rid of the flags altogether and just read the state from the server lookup maps on-demand, but that may be a somewhat larger refactor.
There was a problem hiding this comment.
It passes because nothing calls p.Inbound in those tests. That would race. Maybe the best way to avoid a race is to just cache inbound flag in peer as a bool.
Sorry for being so pedantic about this, the PR is good otherwise.
There was a problem hiding this comment.
@fjl Nah that's not pedantic; I don't want to introduce a race, but I was being fixated on the parts that I used.
Let me make some more tweaks, will ping again.
p2p/server.go
Outdated
| // This channel is used by RemoveTrustedPeer to remove an enode | ||
| // from the trusted node set | ||
| srv.log.Debug("Removing trusted node", "node", n) | ||
| if _, ok := trusted[n.ID]; ok { |
There was a problem hiding this comment.
This check seems to be unnecessary, deleting a non-existent key should not cost more than checking its existence.
shazow
force-pushed
the
addremovetrustedpeer
branch
from
c85443e
to
2086d4f
Compare
|
@fjl I pushed a few changes, but I'm still unable to reproduce the race in testing for whatever reason: E.g. 291fcfb still passes, I tried that block in a separate goroutine also, not sure why it's not getting caught (let me know if you have a suggestion here). I do agree that it should be a race since AddTrustedPeer modifies the flags and p.Inbound() reads the flags. Maybe a bug in the Go race detector where it doesn't notice bitwise operations? I also pushed 4e5f57a which moves the In general, feels like the flags bitwise int should just be a bunch of bools to start with. What was the thinking behind the conn.flags int? |
shazow
force-pushed
the
addremovetrustedpeer
branch
from
b458860
to
ce10c1e
Compare
|
Ah managed to find the right combination of goroutine'ing to get the race conditions to trigger in ce10c1e, will play around with some fixes next. |
|
I ended up wrapping the flag ops with atomic as @zsfelfoldi originally suggested (and rolling back the isInbound cached value for consistency), this makes the failing test pass and is a reasonably small change: 07ef2cc @fjl Please take another look. |
|
Ping. Please take another look. ❤️ |
These RPC calls are analogous to Parity's parity_addReservedPeer and parity_removeReservedPeer. They are useful for adjusting the trusted peer set during runtime, without requiring restarting the server.
shazow
force-pushed
the
addremovetrustedpeer
branch
from
63236c2
to
6209545
Compare
|
Rebased latest master. |
|
Ping ping. |
|
@karalabe @zsfelfoldi is there anything further this PR needs before merging? |
|
@shazow I am sorry, I totally forgot about this and then left for a long vacation. I checked it again, there is one small issue, see my comment. If that it fixed, I think we can merge it immediately. |
| } else { | ||
| flags &= ^f | ||
| } | ||
| atomic.StoreInt32((*int32)(&c.flags), int32(flags)) |
There was a problem hiding this comment.
This is not atomic :) Please use CompareAndSwap here just to be totally safe.
|
@zsfelfoldi Thank you! |
|
Amazing! thanks for the contribution @shazow |
…thereum#16333)" This reverts commit e65b81a.
These RPC calls are analogous to Parity's
parity_addReservedPeerandparity_removeReservedPeer.They are useful for adjusting the trusted peer set during runtime,
without requiring restarting the server.
Also includes a test which should help exercise #16189 as well.