Skip to content

Conversation

@rustaceanrob
Copy link
Contributor

@rustaceanrob rustaceanrob commented May 9, 2025

The actual implementation comes down to listening for an info message that reports the transaction was sent to a peer. For simplicity I am ignoring any wallet updates, but if the user calls the Sync command they can catch them. Follows up #181

Checklists

All Submissions:

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

New Features:

  • I've added tests for the new feature
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

Bugfixes:

  • This pull request breaks the existing API
  • I've added tests to reproduce the issue which are now passing
  • I'm linking the issue being fixed by this PR

@rustaceanrob rustaceanrob changed the title Implement transaction broadcasting Implement transaction broadcasting for Kyoto May 9, 2025
@rustaceanrob rustaceanrob force-pushed the kyoto-broadcast-5-9 branch from c5dd087 to 6f98ece Compare May 9, 2025 08:05
@rustaceanrob rustaceanrob requested a review from notmandatory May 9, 2025 08:07
Copy link
Collaborator

@tvpeter tvpeter left a comment

Choose a reason for hiding this comment

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

Probably my network is poor, I have not been able to successfully broadcast a tx but will keep trying.

Thank you for helping with this.

@rustaceanrob
Copy link
Contributor Author

Probably my network is poor, I have not been able to successfully broadcast a tx but will keep trying.

I am working now to improve the broadcasting reliability in Kyoto, so there is a chance you are running into bugs of mine. However the code here wouldn't change, only internals of the node.

@tvpeter
Copy link
Collaborator

tvpeter commented May 10, 2025

only internals of the node.

Would that require updating bdk-kyoto?

@rustaceanrob
Copy link
Contributor Author

I believe bdk_kyoto would require a patch release as well. I should remark that I have successfully sent transactions with Kyoto over test networks and Bitcoin, so the improvements are more for reliability.

@notmandatory
Copy link
Member

notmandatory commented May 13, 2025

I also was unable to get it to broadcast (I'm using testnet4). It looks like it's never getting to this line:

tracing::info!("Successfully gossiped wtxid: {wtxid}");

This is my test command:

cargo run --features cbf -- --network testnet4 wallet --client-type cbf --database-type sqlite broadcast --psbt cHNidP8BAH0CAAAAAQTsUSSbQmlAXySimNJr91qPEdtAQ2oWSFnFzT2HU6LaAAAAAAD9////AhAnAAAAAAAAIlEgll6PCdGT5N+krKcTBtEXtPDmXWAJpaNnC31A0+2LQGB3eQcAAAAAABYAFF4qFgWGhpAQl6/0r/tSDyKH35qrj0ABAAABAJMCAAAAAfWnu2nwuiC8j50p1f4bBXNVSvOS8hlrvK5v1PEXqcOuAQAAAAD9////AyChBwAAAAAAFgAUWmkCfVH7+qtcUl0UN0qZPDkhmP5QhEkAAAAAABYAFHemwsX9qws0MwQ8kFuUloqvnFnFAAAAAAAAAAAZahdmYXVjZXQudGVzdG5ldDQuZGV2IHR4bgAAAAABAR8goQcAAAAAABYAFFppAn1R+/qrXFJdFDdKmTw5IZj+AQhrAkcwRAIgMdjz8lLAeg+gyQ0RgxdbwD4GnykPeAyjBKmaqFCy2DoCIB+P0wVMkbvjTb8MA0E9Ayb5Kl0UEVXLwRKbvdHz291hASEDIuApJoXbhnB4FRqHzde1fnqo0Wg9L4WBTpWpRocR0aMAAAA=

@rustaceanrob
Copy link
Contributor Author

rustaceanrob commented May 13, 2025

I think I've ironed out what is happening with this test. Because I don't allow for any buffer of time between the node starting and the attempting to send the transaction, Kyoto advertises the transaction sometime during the version handshake with the remote node. As a result the remote node just drops it on the floor. I will change this to broadcast after a short delay, and timeout with an error if the TxGossiped message is not emitted.

@tvpeter
Copy link
Collaborator

tvpeter commented May 13, 2025

Because I don't allow for any buffer of time between the node starting and the attempting to send the transaction, Kyoto advertises the transaction sometime during the version handshake with the remote node. As a result the remote node just drops it on the floor.

That’s exactly what I thought. If there were a way to determine the number of established connections before attempting a broadcast, it would have simplified the process. That is why in the initial implementation, I allowed a 60-second buffer before broadcasting, which made it a bit more reliable.

@rustaceanrob
Copy link
Contributor Author

There is Info::ConnectionsMet, but that is only emitted once the total requirement is met, but it would make the broadcast more reliable. I am going to add a short sleep, then await the TxGossiped with a timeout. If it fails for some reason, the user can try again. Thanks for the observation!

@rustaceanrob rustaceanrob force-pushed the kyoto-broadcast-5-9 branch from 6f98ece to afd6e1b Compare May 13, 2025 10:14
@rustaceanrob
Copy link
Contributor Author

I was able to successfully broadcast a transaction with the most recent commit. I changed the default connections to 2, as there are only a couple CBF nodes on testnet4 and signet

@rustaceanrob rustaceanrob force-pushed the kyoto-broadcast-5-9 branch from afd6e1b to 8ac6957 Compare May 13, 2025 14:01
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

Even with the latest commit I was unable to broadcast a Tx. I did a little debugging and found that I get info_subscriber.len() = 0 right before the while let Some(info) = info_subscriber.recv().await { ... code is called which means it returns None and nothing is ever broadcast.

@rustaceanrob what OS are you using? I'm on MacOS (M1). Could it be some timing difference or way tokio is handling multiple threads? I also tried adding a sleep prior to the above line and it didn't make a difference.

@rustaceanrob
Copy link
Contributor Author

It sounds like your node is returning an error when calling "run". I suggest removing the light_client_data directory. The only time I've gotten errors is 1. the light_client_data directory is in use by another program 2. the wallet is requesting a sync lower than the headers Kyoto is able to load from SQL

@notmandatory
Copy link
Member

You were right! I cleared the light_client_data directory and re-sync'd skipping the first 50K blocks and then I was able to broadcast.

I probably ran into this because the wallet data and light_client_data were in different spots and I probably removed one and not the other during testing, #183 puts them under the same parent. I still think this could use some better error reports to see when issues like this come up, but that can come in future PRs.

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 8ac6957

Please just rebase and fix the build warning and typo and then I'll merge it. Or if you're busy let me know and I'll do it. Thanks!

For the highest reliability, we wait for the connection requirement to
be met by the node. Once met, we can broadcast and wait for
confirmation. The function will either timeout after 15 seconds or
successfully finish with gossip confirmation.
@notmandatory notmandatory force-pushed the kyoto-broadcast-5-9 branch 2 times, most recently from 4aeab92 to d0513e9 Compare May 16, 2025 16:24
@notmandatory notmandatory force-pushed the kyoto-broadcast-5-9 branch from d0513e9 to 811f614 Compare May 16, 2025 16:38
Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK 811f614

@notmandatory notmandatory merged commit 62ff445 into bitcoindevkit:master May 16, 2025
7 checks passed
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.

3 participants