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.

Sign up for GitHub

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

Jump to bottom

Neutrino Wallet: OnCompactFilter handler #905

Merged
merged 8 commits into from
Dec 6, 2019
Merged

Neutrino Wallet: OnCompactFilter handler #905

merged 8 commits into from
Dec 6, 2019

Conversation

rorp
Copy link
Contributor

@rorp rorp commented Dec 2, 2019

No description provided.

@Christewart Christewart added wallet work for the wallet project node work for the node project labels Dec 2, 2019
Christewart
Christewart reviewed Dec 2, 2019
View reviewed changes
core/src/main/scala/org/bitcoins/core/node/NodeApi.scala Outdated Show resolved Hide resolved
node/src/main/scala/org/bitcoins/node/NeutrinoNode.scala Outdated
override def setCallbacks(callbacks: SpvNodeCallbacks): Node =
copy(nodeCallbacks = callbacks)

override def setBloomFilter(bloomFilter: BloomFilter): Node = this
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this method should only be available on SpvNode?

node/src/main/scala/org/bitcoins/node/Node.scala Outdated Show resolved Hide resolved
node/src/main/scala/org/bitcoins/node/SpvNodeCallbacks.scala Outdated
@@ -28,11 +29,16 @@ object SpvNodeCallbacks {
def onMerkleBlockReceived(f: OnMerkleBlockReceived): SpvNodeCallbacks =
SpvNodeCallbacks(onMerkleBlockReceived = Seq(f))

/** Constructs a set of callbacks that only acts on block received */
Copy link
Contributor

Choose a reason for hiding this comment

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

this scaladoc is wrong I think. Should be on compact filter received?

node/src/main/scala/org/bitcoins/node/SpvNodeCallbacks.scala Outdated Show resolved Hide resolved
node/src/main/scala/org/bitcoins/node/networking/peer/DataMessageHandler.scala Outdated
@@ -94,6 +95,12 @@ case class DataMessageHandler(
}
}
newChainApi <- chainApi.processFilter(filter)
_ <- Future {
Copy link
Contributor

Choose a reason for hiding this comment

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

So is this Future needed because the signature is

type OnCompactFilterReceived = (DoubleSha256Digest, GolombFilter) => Unit

rather than returning a Future[Unit]? Are you worried that the callback is going to take a long time to process? It seems like a valid concern to me.

wallet/src/main/scala/org/bitcoins/wallet/Wallet.scala Outdated Show resolved Hide resolved
wallet/src/main/scala/org/bitcoins/wallet/Wallet.scala Outdated Show resolved Hide resolved
@nkohen nkohen added the neutrino Implements the Bitcoin-S Neutrino node label Dec 4, 2019
Christewart
Christewart reviewed Dec 5, 2019
View reviewed changes
node-test/src/test/scala/org/bitcoins/node/NeutrinoNodeWithWalletTest.scala Outdated
}

val condition1 = () => {
condition(0.sats,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you use named arguments (alt+enter on your keyboard in intellij). This makes this kind of stuff so much more readable for code reviewers :-)

node/src/main/scala/org/bitcoins/node/Node.scala Outdated
/**
* Fetches the given blocks from the peers and calls the appropriate [[callbacks]] when done.
*/
def fetchBlocks(blockHashes: Vector[DoubleSha256Digest]): Future[Unit] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

override

node/src/main/scala/org/bitcoins/node/SpvNode.scala Outdated
@@ -30,7 +30,11 @@ case class SpvNode(

override val peer: Peer = nodePeer

override val callbacks: SpvNodeCallbacks = nodeCallbacks
override def setCallbacks(callbacks: NodeCallbacks): Node =
Copy link
Contributor

Choose a reason for hiding this comment

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

is this kind of dangerous? It replaces all existing call backs right? maybe we just want a addCallback()? I would at least add a scaladoc saying it removes existing callbacks and replaces them with the given ones.

testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala Outdated
bitcoind <- BitcoinSFixture.createBitcoindWithFunds(versionOpt)
node <- createSpvNode(bitcoind, callbacks)
fundedWallet <- {
implicit val nodeApi: NodeApi = node
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get rid of this implicit too?

testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala Outdated
bitcoind <- BitcoinSFixture.createBitcoindWithFunds(versionOpt)
node <- createNeutrinoNode(bitcoind, callbacks)
fundedWallet <- {
implicit val nodeApi: NodeApi = node
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as I said above, let's make these explicit calls

testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala Outdated
@@ -113,7 +121,8 @@ object BitcoinSWalletTest extends WalletLogger {
*/
private def createNewWallet(extraConfig: Option[Config])(
implicit config: BitcoinSAppConfig,
ec: ExecutionContext): () => Future[UnlockedWalletApi] =
ec: ExecutionContext,
nodeApi: NodeApi): () => Future[UnlockedWalletApi] =
Copy link
Contributor

Choose a reason for hiding this comment

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

and make this nodeApi a normal parameter rather than implicit one

testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala Outdated
defaultF
}
ec: ExecutionContext,
nodeApi: NodeApi): Future[UnlockedWalletApi] =
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala Outdated
/** Creates a default wallet, and then pairs it with a bitcoind instance that has money in the bitcoind wallet */
def createWalletWithBitcoind()(
implicit system: ActorSystem,
config: BitcoinSAppConfig): Future[WalletWithBitcoind] = {
config: BitcoinSAppConfig,
nodeApi: NodeApi): Future[WalletWithBitcoind] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove implicit nodeApi

testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala Outdated
@@ -177,7 +193,8 @@ object BitcoinSWalletTest extends WalletLogger {
/** Gives us a funded bitcoin-s wallet and the bitcoind instance that funded that wallet */
def fundedWalletAndBitcoind(versionOpt: Option[BitcoindVersion])(
implicit config: BitcoinSAppConfig,
system: ActorSystem): Future[WalletWithBitcoind] = {
system: ActorSystem,
nodeApi: NodeApi): Future[WalletWithBitcoind] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove implicit nodeApi

testkit/src/main/scala/org/bitcoins/testkit/wallet/BitcoinSWalletTest.scala Outdated
def fundedWalletAndBitcoind(bitcoindRpcClient: BitcoindRpcClient)(
implicit config: BitcoinSAppConfig,
system: ActorSystem,
nodeApi: NodeApi): Future[WalletWithBitcoind] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

remove implicit nodeApi

nkohen
nkohen approved these changes Dec 6, 2019
View reviewed changes
Copy link
Collaborator

@nkohen nkohen left a comment

Choose a reason for hiding this comment

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

LGTM

@rorp rorp merged commit a752c53 into bitcoin-s:master Dec 6, 2019
Christewart pushed a commit that referenced this pull request Dec 10, 2019
@benthecarman @Christewart
Rebase PSBT with master (#935)
dd11548 
* Add sanity tests for .hashCode() and .equals() for multiple instances for EclairRpcClient/BitcoindRpcClient (#881)

* Update sbt to 1.3.4 (#889)

* Try to fix mac osx git submodules (#898)

* Add comments indicating what time unit eclair sends things in for the… (#884)

* Add comments indicating what time unit eclair sends things in for the various rpc calls

* Add missing comment

* Make sure time units returned by eclair rpc are correct

* Refactor compact filter matching (#838)

* Refactor compact filter matching

* cleanup

* some more changes

* addressed the PR comments

* cleanup

* cleanup

* Nicer Satoshis.apply (#899)

* Replaced Satoshis(Int64(_)) with Satoshis(_)

* Fixed SignerTest

* Moved BitcoinTxBuilder property tests into ScalaTest context, fixed bug where opPushData was marking valid short P2SH scripts as invalid (#900)

* [Neutrino] Update balances (#888)

* [Neutrino] Update balances

* responded to the comments

* some more changes

* Make 'downloadBitcoind' command async, which reduces task time by 3x (#906)

* Add descriptors as toString for script pubkeys (#902)

* BitcoinTxBuilderTest Hang (#901)

* Fix hanging bug in BitcoinTxBuilderTest

* Added forAllAsync to BitcoinSAsyncTest

* Update akka-http, akka-http-testkit to 10.1.11 (#908)

* Add documentation on env variable SBT_OPTS and add documentation about it (#914)

* 2019 11 28 cli native image doc (#903)

* Add documentation about how the cli can be built with a native image

* Finish rest of documentation on building command line client for bitcoin-s

* Add missing akka actor on walletServer classpath, add plugins to be able to build the walletServer as a standalone jar

* Add server.md

* Rename walletServer -> appServer

* Address code review, add missing main class in server project

* Address code review

* 2019 11 30 key manager (#904)

* Segregate key manager from wallet

* More consistent naming in build.sbt, run scalafmt

* Add test case making sure we don't overwrite a mnemonic seed

* Add key-manager.md

* Fix compile issues with older scala versions

* Address code review

* Bitcoind v19 RPC (#910)

* bitcoind v19 new RPC calls and tests (#863)

* bitcoind v19 new RPC calls and tests

* Code review changes

* Review part 2

* Rename variable to be more descriptive

* Explanitory comment

* Ignore broken test cases

* Add missing signing functions

* Add test to check avoid_reuse flag is on (#870)

* Add test to check avoid_reuse flag is on

* Add test to make sure flags weren't set

* bitcoind v19 Update mempool RPCs and tests (#868)

* Update mempool RPC calls to bitcoind v19 compatibility

* Typo fix

* Add parameter name to calls

* Fix remaining rpc calls

* Formatting

* scaladoc for param

* Change param to correct type

* Clarify on scaladoc

* Add missing fees parmater to mempool rpcs (#875)

* Add weight field to mempool entries after v19 (#876)

* Move DescriptorRpc to be able to be used by future versions of bitcoind (#878)

* Add window_final_block_height to GetChainTxStatsResult (#880)

* Add passphrase argument to createwallet for later versions (#883)

* Add passphrase argument to createwallet for later versions

* Scaladoc + empty passphrase requirement

* Error message

* Add new services names parameter to P2P rpcs (#874)

* Add new services names parameter to P2P rpcs

* Add ServiceIdentifier Reads

* Add fallback case

* Address review

* Change to Try

* Move PsbtRpc to be able to be used by future versions of bitcoind (#877)

* Move PsbtRpc to be able to be used by future versions of bitcoind

* Add test

* Address comment

* Enable bloom filters for v19

* Enable bip 61 for tests

* Change to official binaries

* Force v18 for Spv Tests

* Remove unused config line

* Timelock bug fix (#920)

* Fix CLTV bug where timestamps using seconds are not allowed in TxBuilder

* Added test for CLTV second timestamp and ran scalafmt

* Fixed another bug

* Fixed bug where calcLockTime was not looking past first CLTV tx

* Neutrino Wallet: OnCompactFilter handler (#905)

* Neutrino Wallet: OnCompactFilter handler

* ChainQueryApi (#926)

* ChainQueryApi

* cleanup

* BIP32Path Factory and new tests (#928)

* Add little endian functionality to Network Element and Factory (#931)

* 2019 12 10 scalatest 3.1.0 (#933)

* Update scalatest to 3.1.0

* Modify code to support scalatest 3.1.0
Christewart pushed a commit that referenced this pull request May 1, 2021
@rorp
Neutrino Wallet: OnCompactFilter handler (#905)
c268a75 
* Neutrino Wallet: OnCompactFilter handler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Christewart Christewart Christewart left review comments

@nkohen nkohen nkohen approved these changes

Assignees
No one assigned
Labels
neutrino Implements the Bitcoin-S Neutrino node node work for the node project wallet work for the wallet project
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rorp @Christewart @nkohen