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

Increase scoring params #3483

Closed
wants to merge 1 commit into from

Conversation

tankyleo
Copy link
Contributor

@tankyleo tankyleo commented Dec 12, 2024

See commit msg for a description - hope this helps with the 0.1 release.

I am hesitant about the 0.1% number for the multiplier default that this PR currently sets - maybe should be halved to 0.05% ? Happy to tweak further.

Closes #3040

Multiply liquidity penalties by 15, as recommended by benthecarman in
issue lightningdevkit#3040.

Set `base_penalty_msat` to 1000: for a 1 sat payment we are willing
to pay 1 satoshi to avoid routing through a specific channel.

Set `base_penalty_amount_multiplier_msat` to 1e6: we are willing to
spend 0.1% of the total amount sent over a particular channel to avoid
routing through that channel. amt * 0.1 / 100 ~= 1e6 * amt / 2^30.

Fix lightningdevkit#3040
Copy link
Contributor

@shaavan shaavan left a comment

Choose a reason for hiding this comment

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

Thanks for taking it up! 🚀

liquidity_penalty_multiplier_msat: 30_000,
liquidity_penalty_amount_multiplier_msat: 192,
base_penalty_msat: 1000,
base_penalty_amount_multiplier_msat: 1_000_000,
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a bit hefty base_penalty_amount_multiplier_msat, isn't it? :)
https://github.com/MutinyWallet/mutiny-node/blob/68d545d1f82ba0d0e64980d056dc16ee7f00d457/mutiny-core/src/node.rs#L1944

Suggested change
base_penalty_amount_multiplier_msat: 1_000_000,
base_penalty_amount_multiplier_msat: 8192 * 100,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

8192 * 100 ~= 1_000_000 if you squint hard enough :)

See the commit message for the reasoning - would I pay an extra 0.1% of current total flow over a channel for better routing ? yes.

@@ -488,7 +488,7 @@ where L::Target: Logger {
pub struct ProbabilisticScoringFeeParameters {
/// A fixed penalty in msats to apply to each channel.
///
/// Default value: 500 msat
/// Default value: 1000 msat
Copy link
Contributor

Choose a reason for hiding this comment

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

@TheBlueMatt

Should we use this opportunity to define constants for each parameter?
This could simplify documentation maintenance and allow us to define constants cleanly using multiplier syntax.

Something like:

const BASE_PENALTY_MSAT: u64 = 100 * 1000;
const BASE_PENALTY_AMOUNT_MULTIPLIER_MSAT: u64 = 8192 * 100;
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ben showed the operands to show how much mutiny deviated from the defaults - I think we should set a single number for the default settings.

@tankyleo
Copy link
Contributor Author

Close in favor of incoming PR from Matt.

@tankyleo tankyleo closed this Dec 12, 2024
@tankyleo tankyleo deleted the scoring-increase branch December 12, 2024 21:38
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.

Rework/increase scoring params
2 participants