Introduction of AttemptClass and PaymentClass (Issue #20) #24
Conversation
first stub of a method as described in renepickhardt#16 Still needs to be tested
… routed successfully
This reverts commit 8dffa28.
There was a problem hiding this comment.
Thanks a lot! Despite me having a few question mark, comments and suggestions (I guess it is much easier to question code rather than writing it) it is a real pleasure reading your code. I have the feeling you write much cleaner code than I do!
strong concept ACK!
pickhardtpayments/Attempt.py
Outdated
| def __init__(self, path: list[Channel], amount: int = 0): | ||
| """Constructor method | ||
| """ | ||
| self._paths = None |
| for channel in path: | ||
| if not isinstance(channel, Channel): | ||
| raise ValueError("path needs to be a collection of Channels") | ||
| self._path = path |
There was a problem hiding this comment.
what is the difference to self._paths from line 32?
There was a problem hiding this comment.
this is the real one ;)
pickhardtpayments/Attempt.py
Outdated
| self._amount = amount | ||
|
|
||
| def __str__(self): | ||
| return "Path with {} channels to deliver {} sats and status {}.".format(len(self._path), |
There was a problem hiding this comment.
Looking how I depict attempts in the evaluation we might want to change that string to include stuff like probability, fees, ppm. We could get inspiration by the printoutput of the simulation
pickhardtpayments/Attempt.py
Outdated
| for attempt in attempts: | ||
| if not isinstance(attempt, Channel): | ||
| raise ValueError("path needs to be a collection of Channels") | ||
| self._paths = attempts |
There was a problem hiding this comment.
I really don't get this :(
There was a problem hiding this comment.
this is indeed unnecessary as well.
This is part of the setter for path in Attempt. When I originally added the path to attempt, I used this setter. And while I refactored step by step, at the time i wasn't sure that didn't make any mistakes when passing on parameters. That's why i checked before inclusion and raised errors to see where things failed.
The setter now can go, because I made this a mandatory parameter at instantiation and I require the correct class.
Thanks for pointing this out!
| :return: estimated success probability before the attempt | ||
| :rtype: float | ||
| """ | ||
| return self._probability |
There was a problem hiding this comment.
not entirely sure as the attempt class does not know the uncertainty network but we could compute this on the fly for the path (as the path contains such information)
There was a problem hiding this comment.
there is btw a strong attempt against it as non disjoint paths will change probability computation.
There was a problem hiding this comment.
yes, this is to collect statistical a priori information:
In _estimate_payment_statistics this is added when calling uncertainty_network.get_features_of_candidate_path. it's tied to this instance of attempt, so another Attempt on the same graph will have its own (then perhaps different probability). It should be purely descriptive and is not used for further calculation.
There was a problem hiding this comment.
Bias in probabilities in UncertaintyGraph?
When collecting possible paths in a round/"payment loop", all path candidates are assumed to be sent and settled successfully.
This is my assumption, because we iterate over each attempt:
UncertaintyChannel.success_probabilityfreshly calculates probabilities from current channel balances, and- then
allocate_amount_on_pathis updated before the loop commences with the next possibly not disjoint path, that then gets its probabilities from the just before adjusted inflight amount.
Or am I reading it wrong?
Ist this something we can live with?
What would be possible ways to correct for this (like trying to immediately figure out if the onion would fail or not)?
There was a problem hiding this comment.
This indeed the really messy book keeping that I introduced. I am happy to find a better solution to this! I need to allocate the amounts for planned onions to have probability correction of the next path correct. (Ask me for an example if you need one!) I remove all of the allocated HTLCs then before I do the actual sending / probing. It is really the most ugly part of my code :(
There was a problem hiding this comment.
just started to look at the code. I think this is btw exactly the difference between PLANNED and INFLIGHT while internally I increase the inflight counter I am actually in the planning phase and to compute the probabilities of planned paths I need to take into account that I already plan to allocate something sats to an edge (without knowing if that will work yet) So maybe we just need to move over the logic from the Enum to the state of a channel where we also have planned sats before we go and send onions and switch those to inflight sats. in this way I would note have to remove them before testing onions. does that make any sense? I am actually not sure myself if this would be any good.
| inflight_attempts = [] | ||
| failed_attempts = [] | ||
| print("\nStatistics about {} candidate onions:\n".format(len(attempts))) | ||
| for attempt in attempts: |
There was a problem hiding this comment.
I guess this is motivated by how the code was histroically structured. But wouldn't it make sense to have this attempts list as an sub_paymet = Payments() object and later in the loop just add he sub_payment to the payment?
There was a problem hiding this comment.
this would allow us that we could later call something like sub_payment.filter(SettlementStatus.FAILED) instead of using an additional list
pickhardtpayments/Attempt.py
Outdated
| from pickhardtpayments import Channel | ||
|
|
||
|
|
||
| class SettlementStatus(Enum): |
| if success: | ||
| attempt.status = SettlementStatus.INFLIGHT |
There was a problem hiding this comment.
wouldn't that be AttmptStatus.ARRIVED?
| src, dest, amt, mu, base) | ||
| # transfer to a min cost flow problem and run the solver | ||
| # paths is the lists of channels, runtime the time it took to calculate all candidates in this round | ||
| attempts_in_round, runtime = self._generate_candidate_paths( |
There was a problem hiding this comment.
maybe sub_payment of type Payment
| end = time.time() | ||
| # When residual amount is 0 / enough successful onions have been found, then settle payment. Else drop onions. | ||
| if amt == 0: | ||
| for onion in payment.inflight_attempts: |
There was a problem hiding this comment.
technically inflight might not be at recipient yet and could still fail. So I guess we want ARRIVED here. I guess that was the confusion of this artificial distinction between INFLIGHT and ARRIVED
There was a problem hiding this comment.
Yes. I think that indeed it is a lot clearer with ARRIVED.
…lative paths on imports
| :return: estimated success probability before the attempt | ||
| :rtype: float | ||
| """ | ||
| return self._probability |
There was a problem hiding this comment.
just started to look at the code. I think this is btw exactly the difference between PLANNED and INFLIGHT while internally I increase the inflight counter I am actually in the planning phase and to compute the probabilities of planned paths I need to take into account that I already plan to allocate something sats to an edge (without knowing if that will work yet) So maybe we just need to move over the logic from the Enum to the state of a channel where we also have planned sats before we go and send onions and switch those to inflight sats. in this way I would note have to remove them before testing onions. does that make any sense? I am actually not sure myself if this would be any good.
pickhardtpayments/Attempt.py
Outdated
| def __init__(self, path: list[UncertaintyChannel], amount: int = 0): | ||
| """Constructor method | ||
| """ | ||
| self._routing_fee = -1 |
There was a problem hiding this comment.
Didn't we say None instead of -1? Note that in particular one might have negative routing fees in the protocol in the future.
There was a problem hiding this comment.
yes, negative routing fees will indeed break the -1 idea. I only changed it in payment, now changed it in attempt as well.
I would actually extract the planned/inflight problem into its own issue. i fear that discussion around it will get lost in this Thread. -> #25
| self._probability *= channel.success_probability(amount) | ||
| # When Attempt is created, all amounts are set inflight. Needs to be updated with AttemptStatus change! | ||
| # This is to correctly compute conditional probabilities of non-disjoint paths in the same set of paths | ||
| # channel.in_flight(amount) |
There was a problem hiding this comment.
So far so good and makes sense
| """ | ||
| if not self._status == value: | ||
| # remove allocated amounts when Attempt status changes from PLANNED | ||
| if self._status == AttemptStatus.PLANNED and not value == AttemptStatus.INFLIGHT: |
There was a problem hiding this comment.
Why not inflight?
I am wondering if we should have a setter API or if we can only inc status by 1.
That being said I see the idea. While i think it is -given the design of the code- the only reasonable option i think it demonstrates that I messed something up. (It feels really wrong to do the amounts tweeking here)
There was a problem hiding this comment.
Concept ACK. Thank you very much for this excellent work and for your endurance!
I think we are on the brink of merging this and refactoring the rest in #18 . However I had a few things where I have the feeling that the refactoring has not been executed fully. I hope my comments are helpful (as mentioned I have really poor concentration today)
Some of the comments don't need intermediate action but are just for the follow up PR but since we still need some tiny changes I would kindly ask you to also rebase against the main branch so that I can hopefully merge tomorrow (:
sebulino
force-pushed
the
payments_attempts
branch
from
2618078 to
62296de
Compare
pickhardtpayments/Payment.py
Outdated
| except ValueError: | ||
| logging.warning("ValueError in Payment.filtered_attempts") | ||
| return [] | ||
| return [attempt for attempt in self._attempts if attempt.status.value == flag.value] |
There was a problem hiding this comment.
Haha! That is not what I meant by creating a generstor using yield. But it is great to see how python can reduce "trivial" code blocks. (:
|
Also LGTM! |
sebulino
force-pushed
the
payments_attempts
branch
from
0ea743d to
111a691
Compare
|
It seems like this change broke the payment algorithm or simulation. PickhardtPaymentsExample.ipynb examples don't work anymore: Round number: 10 Statistics about 46 candidate onions: successful attempts:p = 100.00% amt: 216543 sats hops: 4 ppm: 1413 failed attempts:p = 100.00% amt: 88000 sats hops: 2 ppm: 1339 SUMMARY:Rounds of mcf-computations: 10 |
…hardt#20) (renepickhardt#24)" This reverts commit cd46871.
…hardt#20) (renepickhardt#24)" This reverts commit cd46871.
Extraction of Attempt and Payment.
A Payment is a central object in the library, and in particular in the simulation. This is why it "deserves" its own Class.
Each onion is recorded as an Attempt and registered with the respective Payment. This not only allows for statistics for each Payment object but also collects all attempts where an amount could successfully flow from sender to receiver.
refactoring of
SyncSimulatedPaymentSessionhas been done as well as a first version of the documentation of the two classes.