Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

neutrino: Added unban peer feature #270

Closed
wants to merge 32 commits into from

Conversation

Chinwendu20
Copy link
Contributor

@Chinwendu20 Chinwendu20 commented Apr 18, 2023

Changes:

  • Added unbanpeer method to chainservice.
  • Added unbanipnet method to banstore

Associated PR in LND: lightningnetwork/lnd#7606 (With detailed explanation on how to test with LND)
Fixes issue: #253

@Chinwendu20 Chinwendu20 changed the title [WIP]neutrinokitserver: Added unban peer feature [WIP]neutrino: Added unban peer feature Apr 18, 2023
@Chinwendu20 Chinwendu20 marked this pull request as draft April 18, 2023 06:36
@Chinwendu20 Chinwendu20 changed the title [WIP]neutrino: Added unban peer feature neutrino: Added unban peer feature Apr 19, 2023
@Chinwendu20 Chinwendu20 marked this pull request as ready for review April 19, 2023 18:03
neutrino.go Show resolved Hide resolved
@Chinwendu20 Chinwendu20 requested a review from Roasbeef May 3, 2023 08:05
@Roasbeef
Copy link
Member

Has a linter error failure:

neutrino.go:1085: unnecessary leading newline (whitespace)
	defer func() {

@Chinwendu20
Copy link
Contributor Author

Hello @Roasbeef I have fixed that

@positiveblue positiveblue self-requested a review May 18, 2023 14:10
Copy link

@positiveblue positiveblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left one question about adding the conn to the connManager when we fail to get the tcpAddr but the rest looks good 🚀

Also, we should test it. Maybe we can simply update the banPeer test.

neutrino.go Show resolved Hide resolved
banman/store.go Outdated Show resolved Hide resolved
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is only missing a test now as jorid noted.

@@ -66,6 +66,9 @@ type Store interface {

// Status returns the ban status for a given IP network.
Status(*net.IPNet) (Status, error)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In commmit message: banmen -> banman

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh okay thanks.

@Chinwendu20
Copy link
Contributor Author

Chinwendu20 commented Jun 14, 2023

Okay I would write the test.
I can write test for the newly added method in store.go but for the newly added chainservice method. I see that chainservice methods do not have unit tests written for them, there is no neutrino_test.go file.

@lightninglabs-deploy
Copy link

@positiveblue: review reminder
@Chinwendu20, remember to re-request review from reviewers when ready

guggero and others added 10 commits August 22, 2023 12:46
We can't re-create the pkScript from a keyspend p2tr witness, as we're
missing the public key. So this warning will trigger for each block that
contains a p2tr key spend, which isn't really useful and confuses users.
So we're downgrading the error to a debug message.
The rebroadcaster has now the ability to be supplied with a
custom error mapping function which should map different backend
errors to the neutrino internal broadcast error.
In this commit, the worker manager is adjusted to only retry a request
a certain maximum number of times. This is done so that a smooth
transition can be made from call sights currently using the
neutrino.queryChainServicePeers method to using the query.WorkManager
instead so that the same default behaviour can be expected. In order to
also cater for call sights currently using the query.WorkManager, a
NoRetryMax optional paramater is added that can used to disable the
retry maximum.
Introduce a `WorkManager` interface that encapsulates the Dispatcher
interface along with `Start` and `Stop` methods. The previous
`WorkManager` struct is converted to `workManager` which is an
implementation of the new `WorkManager` interface. The `ChainService`
struct is then adapted to hold the new interface instead of the struct.
This will make testing easier in future.
Use the work dipatcher interface from the query package to make getdata
requests instead of using the old queryPeers function.
Rename the `query` variable in `GetCFilter` so that the `query` package
can be imported and used cleanly.
Use the work dispatcher query interface instead of the old queryPeers
method for making getcfilter requests. This ensures that the queries are
made to the most responsive peers.
Since both GetBlock and GetCFilter now make use of the work dispatcher
instead of the `queryPeers` function, we can now remove the `queryPeers`
and `queryChainServicePeers` functions.
ellemouton and others added 22 commits August 22, 2023 12:46
Lint the files and update the tests to use the `require` package.
Add a new FilterData type that encapsulates the filter, blockhash and
type associated with a filter. This is done in order to prepare for a
commit that will enable batch writes of these filters.
Introduce a new `rescanState` struct which holds the variables required
for a rescan. We then also conver the existing `rescan` function into a
method of `rescanState`. This refactor will assist in making future
clean up of the `rescan` method easier.
Add a new `waitForBlocks` helper method to rescanState that will wait on
block notifications until a given predicate returns true. Currently this
method is only called with one predicate but an upcoming commit will
make use of it using a differnet predicate.
This is required for the follow up commit where access to the IsCurrent
method is required in the rescanState.rescan method which only has
access to the ChainSource interface.
Let the rescan function wait until the filter headers have either caught
up to the back end chain or until they have caught up to the specified
rescan end block. This lets the rescan operation take advantage of doing
batch filter fetching during rescan making the operation a lot faster
since filters can be fetched in batches of 1000 instead of one at a
time.
This check was performed for both the reorg and non-reorg cases, but
in the reorg case the headers would count for PoW before validating
that the headers connected to one another.
This uses btcd's HeaderCtx and ChainCtx interfaces to be able to
validate headers, both contextually and context-free. This allows
neutrino to call blockchain.CheckBlockHeaderContext and
blockchain.CheckBlockHeaderSanity.
@Chinwendu20
Copy link
Contributor Author

Hello, the commit issues here, I do not know how to fix them, so I would be creating a new PR for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants