-
Notifications
You must be signed in to change notification settings - Fork 353
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
Improve block connection logging #2559
Conversation
|
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2559 +/- ##
==========================================
- Coverage 90.57% 89.04% -1.53%
==========================================
Files 110 112 +2
Lines 57557 86947 +29390
Branches 57557 86947 +29390
==========================================
+ Hits 52130 77419 +25289
- Misses 5427 7296 +1869
- Partials 0 2232 +2232
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of merging the upstream code into your PR, please rebase onto upstream if there is a conflict. If there's not actually a conflict, please don't bother.
for tx in txdata.iter() { | ||
let (index, transaction) = tx; | ||
let txid = transaction.txid(); | ||
log_trace!(self.logger, "Transaction id {} confirmed in block {}", txid, block_hash); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we should log every transaction, as many users actually connect all transactions in a block. Instead, we should look for transactions we care about, and maybe only actually add logging in ChannelMonitor where we can do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already filter block data at the top of transactions_confirmed
, so you can print for everything in txn_matched
I have updated the changes,please review |
Instead of merging the upstream code into your PR, please rebase onto upstream. Merging the upstream branch via git merge/git pull makes review very difficult as its not clear what the actual head commit is. |
The issue also mentions "maybe also log when we add things to the filter." which I think we should absolutely do here. |
Can you also log what's going on at the top of |
I will keep in mind from my next PR. |
I have updated the changes please review :) |
Thanks for working on this @Sharmalm |
There are several conditions that can be violated which indicate a stale monitor. This logs each that doesn't hold.
- Add fuzz test for `NetAddress` `from_str` function
We upstream the `KVStore` interface trait from LDK Node, which will replace `KVStorePersister` in the coming commits. Besides persistence, `KVStore` implementations will also offer to `list` keys present in a given `namespace` and `read` the stored values.
We add a utility function needed by upcoming `KVStore` implementation tests.
We upstream the `FilesystemStore` implementation, which is backwards compatible with `lightning-persister::FilesystemPersister`.
This replaces the `FilesystemPersister::read_channelmonitors` method, as we can now implement a single utility for all `KVStore`s.
Firstly, we switch our BP over to use `FilesystemStore`, which also gives us test coverage and ensures the compatibility. Then, we remove the superseded `KVStorePersister` trait and the `FilesystemPersister` code.
We re-add benchmarking for `FilesystemStore` now that we switched over to it.
When an invoice is requested but either receives an error or never receives a response, surface an event to indicate to the user that the corresponding future payment has failed.
When a BOLT 12 invoice has been requested, in order to guarantee invoice payment idempotency the future payment needs to be tracked. Add an AwaitingInvoice variant to PendingOutboundPayment such that only requested invoices are paid only once. Timeout after a few timer ticks if a request has not been received.
When a BOLT 12 invoice has been received, a payment attempt is made and any errors result in abandoning the PendingOutboundPayment. This results in generating at PaymentFailed event, which has a PaymentHash. Thus, when receiving an invoice, transition from AwaitingInvoice to a new InvoiceReceived state, the latter of which contains a PaymentHash such the abandon_payment helper can still be used.
Consolidate the creation and insertion of onion_session_privs to the PendingOutboundPayment::Retryable arm. In an upcoming commit, this method will be reused for an initial BOLT 12 invoice payment. However, onion_session_privs are created using a different helper.
It will be used for initial attempts at paying BOLT 12 invoices, so rename it something that covers both that and retries. Support paying BOLT 12 invoices Add a send_payment_for_bolt12_invoice method to OutboundPayments for initiating payment of a BOLT 12 invoice. This will be called from an OffersMessageHandler, after which any retries are handled using the Retryable logic. pub(crate) visibility for offers/test_utils.rs The test utilities for Offers are needed for testing message handling in ChannelManager and OutboundPayments. Add tests for send_payment_for_bolt12_invoice Use u32 instead of usize in Retry::Attempts An upcoming commit requires serializing Retry, so use a type with a fixed byte length. Otherwise, using eight bytes to serialize a usize would fail to read on 32-bit machines. Configure BOLT 12 invoice payment retry strategy Replace a constant three retry attempts for BOLT 12 invoice payments with a retry strategy specified when creating a pending outbound payment. This is configured by users in a later commit when constructing an InvoiceRequest or a Refund. Rename SocketAddress from NetAddress
Thanks for all support and guidance from mentors. |
@jbesraa is rebasing correct? |
unfortunately its not, you should endup with a single commit in this pr. |
Ok So , should I rebase all the above commits so that there should be one commit left in PR |
yes, this might be heplful https://stackoverflow.com/questions/17577409/git-remove-merge-commit-from-history. and this https://stackoverflow.com/questions/16741724/does-pulling-from-a-remote-give-the-branches-a-different-history could be a good read to understand merge vs rebase |
Hello , I think i messed up with branches and rebasing , so i will close this PR and make a new PR . |
this pr solves #2348.