Skip to content

Conversation

stevenroose
Copy link
Contributor

No description provided.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Nice! I've been meaning to add something like this for a while now. My only substantial comment is to adequately detail the two HTLC handling modification options which have been put into place for testing purposes.

sample-lnd.conf Outdated
Copy link
Member

Choose a reason for hiding this comment

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

A comment should be added to detail that this option and the one above it exist primarily for testing purposes.

In the case of debughtlc it uses the same payment hash each time. This means that intermediate nodes that see this payment hash can instantly take the funds locked within the output during the clearing phase, skipping forwarding the HTLc to any other participants.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should just skip the ones meant for testing? Testers know them anyway and it can only be confusing and take up space.

Copy link
Contributor Author

@stevenroose stevenroose Nov 1, 2017

Choose a reason for hiding this comment

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

I would remove

  • debughtlc
  • hodlhtlc
  • simnet
  • regtest

to be honest.

Copy link
Member

Choose a reason for hiding this comment

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

simnet and regtest are fine as developers may be using that locally for testing purposes. It's just the two HTLC modifiers that are only used within our automated integration tests

sample-lnd.conf Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit: a new line should be added above.

@stevenroose
Copy link
Contributor Author

Updated.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🤾🏻‍♀️

@Roasbeef Roasbeef merged commit 4e615f9 into lightningnetwork:master Nov 12, 2017
@stevenroose
Copy link
Contributor Author

It looks like the conf file is not really parsed as TOML.. In my setup it seems to require the section prefix explicitly.

[Bitcoin]
active=1

Returns either bitcoin.active or litecoin.active must be set to 1 (true)

While

[Bitcoin]
bitcoin.active=1

works.

Is this a TOML parsing problem or does it look for a command line flag explicitly? (Can look into this tonight after work.)

@Roasbeef
Copy link
Member

That's because we add the namespace struct stags to each of the fields as well. This is what enables users to do --bitcoin.active on the command line.

@stevenroose
Copy link
Contributor Author

In TOML, the [header] should determine the "namespace", no? Let me look into this tonight.

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.

2 participants