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

nakamoto as Compact Block Filter Blockchain #751

Closed
wants to merge 6 commits into from

Conversation

rajarshimaitra
Copy link
Contributor

@rajarshimaitra rajarshimaitra commented Sep 13, 2022

Description

An implementation of nakamoto as a BDK Blockchain to serve as an alter BIP157 syncing mechanism.

Commit 1 : adds a new module src/blockchain/compact_filters/nakamoto.rs which defines a CbfNode struct which holds the namakoto-client handlers. Which the implements Blockchain traits. 222a2b4

Commit 2 : Integrates the CbfNode backend with the bdk's blockchain test framework.

Notes to Reviewers

The current CI tests won't work because nakamoto doesn't support our MSRV. Though you can run the modified tests in your local with toolchain 1.63 or higher.

Few of the tests are intermittently failing and needs further investigation. But overall this PR is ready for review of to be used for testing..

Thanks for all the help and support from @cloudhead and others who pitched in for various fixes in upstream.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

@rajarshimaitra rajarshimaitra mentioned this pull request Sep 14, 2022
9 tasks
@rajarshimaitra rajarshimaitra marked this pull request as ready for review October 8, 2022 17:42
@rajarshimaitra rajarshimaitra changed the title [WIP] nakamoto as Compact Block Filter Blockchain nakamoto as Compact Block Filter Blockchain Oct 8, 2022
Copy link

@cloudhead cloudhead left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Cargo.toml Outdated
nakamoto-net = { git = "https://github.com/cloudhead/nakamoto.git", branch = "master", optional = true }
nakamoto-net-poll = { git = "https://github.com/cloudhead/nakamoto.git", branch = "master", optional = true }
nakamoto-p2p = { git = "https://github.com/cloudhead/nakamoto.git", branch = "master", optional = true }
nakamoto-common = { git = "https://github.com/cloudhead/nakamoto.git", branch = "master", optional = true }

Choose a reason for hiding this comment

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

You should be able to just import the nakamoto crate. It exposes these externally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip.. I missed it.. Yes that's much more cleaner..

transactions,
} => {
let timestamp = header.time;
for tx in transactions {

Choose a reason for hiding this comment

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

Note that all transactions of the matching block are included here, so you have to make sure to discard the ones you aren't interested in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are actually handling that in the add_tx itself.. So any unrelated transaction will not get added in the db.. But ya the will be iterated over here but with not much extra over head as some basic if let statements in add_tx won't trigger for them. Maybe there's a better way to organize this, but for start it made sense to consolidate all transaction conflicts and addition logic in single place in add_tx..

Here we are just checking if the transaction hasn't been already processed by us..

.collect::<HashSet<_>>();

for txid in db_txs {
unconfirm_tx(database, &txid)?;

Choose a reason for hiding this comment

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

Note that nakamoto should already be emitting a TxStatus::Reverted for each tx in a reverted block, so you shouldn't have to handle it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm okay makes sense.. We still handle reverted cases in TxStatus::Reverted.. I am commenting this section out for now..

debug!("Sync complete at : {}", height);
self.last_sync_height.set(height as u32);
break;
}

Choose a reason for hiding this comment

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

I assume you have another mechanism to restart the event loop when a new block header is received.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this is basically a hack to stop the sync loop for the reorg tests in regetst.. Ideally an invalidateblock call on the regtest should also update the tip height, and the sync should automatically stop at the new tip height, but that wasn't happening.. So instead this hack is to force stop the sync.. This is feature gated as test, and cannot be used in production..

Choose a reason for hiding this comment

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

Ah gotcha, ok.

}
TxStatus::Reverted => {
debug!("Transaction reverted due to reorg: txid : {}", txid);
let _ = unconfirm_tx(database, &txid)?;

Choose a reason for hiding this comment

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

FYI, nakamoto will re-announce reverted transactions. I don't think we fire another event for them other than this one (but we could).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this transaction reversion event should be sufficient to catch them..

@rajarshimaitra
Copy link
Contributor Author

Thanks @cloudhead for the review and suggestion.. It seems the structure of the crate has changed in recent master.. So I pinned the rev to 14902d3d5cc9442cdda82419f0f24277d86b243b. I couldn't find any release tag in github, and the published v0.3.0 in crates.io doesn't seem to include some latest fixes.. Anyway, this works for now, until we get a more stable release of nakamoto out in crates.io..

@rajarshimaitra
Copy link
Contributor Author

rajarshimaitra commented Oct 12, 2022

@thunderbiscuit this now includes AnyBlockhain and ConfigurableBlockchain updates..

@rajarshimaitra rajarshimaitra marked this pull request as draft October 12, 2022 07:18
Copy link
Member

@thunderbiscuit thunderbiscuit left a comment

Choose a reason for hiding this comment

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

Hey Raj! So I'm trying to expose this in the bindings and it surfaces two small things: (1) I think the names for the other blockchain config structs are slightly different, (2) the fields in the config struct are currently private but I'd need access to them.

To get an idea for how we use the config structs (and how we wrap them up slightly, note the ElectrumBlockchainConfig to ElectrumConfig same for the Esplora) you can take a look at lines 98ish to 225ish of this source file.

src/blockchain/compact_filters/nakamoto.rs Outdated Show resolved Hide resolved
src/blockchain/compact_filters/nakamoto.rs Outdated Show resolved Hide resolved
@rajarshimaitra
Copy link
Contributor Author

@thunderbiscuit updated as suggested..

@rajarshimaitra
Copy link
Contributor Author

One more extra commit is added to disable to existing compact_filters logic, so we don't need to unnecessarily build rocksdb and other stuffs not used for nakamoto client..

@thunderbiscuit
Copy link
Member

thunderbiscuit commented Oct 13, 2022

Working on the FFI integration here: bitcoindevkit/bdk-ffi#207
With this wallet: Devkit Wallet Nakamoto branch

This commit adds the nakamoto blockchain into the current blockchain test
frameworks. Uses a lot of feature gate and scaffolding to handle special
behaviors of CBF type backend.
Include CbfNode in AnyBlockchain enum
Impl ConfigurableBlockchain for CbfNode
This commit comments out existing compact filters module so only
nakamoto dependencies get compiled with compact_filters features.
@danielabrozzoni
Copy link
Member

Hey, we are in the process of releasing BDK 1.0, which will under the hood work quite differently from the current BDK. For this reason, I'm closing all the PRs that don't really apply anymore. If you think this is a mistake, feel free to rebase on master and re-open!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

5 participants