Skip to content

Conversation

@harding
Copy link
Collaborator

@harding harding commented Nov 25, 2018

No description provided.


- C-Lightning [#2081][c-lightning #2081] and [#2092][c-lightning #2092]
fix a problem with running multiple RPC commands in parallel. As a
user-visible change, lightningd now adds a double newline (`\n\n`)

Choose a reason for hiding this comment

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

Should be lightningd instead fo lightningd to make it consistent with how bitcoind is written below? :-)

answers made since our last update.*

- [How could you create a fake signature to pretend to be
Satoshi?][bse 81115] Gregory Maxwell asks and answers a question about

Choose a reason for hiding this comment

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

Trailing space on this line :-)

previously silently ignore the testnet options. This merged PR causes
it to print a notice: "Warning: Section [testnet] is not recognized."


Choose a reason for hiding this comment

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

Nit: Multiple consecutive blank lines – not needed in Markdown :-)


Wuille suggests two additions to what metadata is hashed: first, the
transaction fee is included in the hash in order to ensure hardware
wallets or offline wallets can be absolutely sure that they aren't

Choose a reason for hiding this comment

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

Style nit: Would changing from "absolutely sure" to "sure" change the meaning? :-)

Copy link
Contributor

@jnewbery jnewbery left a comment

Choose a reason for hiding this comment

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

ACK 292e8c6

Looks great. A few minor style nits inline. Feel free to ignore.


## Action items

- **Monitor feerates:** recent reductions in the exchange rate have lead
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm usually reluctant to assign a cause to short-run hashrate changes, but you're almost certainly right here, so I think it's fine to leave this. I would qualify 'the exchange rate' though, perhaps 'the BTC-USD exchange rate' or similar?

Wuille suggests two additions to what metadata is hashed: first, the
transaction fee is included in the hash in order to ensure hardware
wallets or offline wallets can be absolutely sure that they aren't
being tricked into sending excess amounts of money to miners.
Copy link
Contributor

Choose a reason for hiding this comment

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

'excess amounts of money' sounds a little odd to me. I think 'sending excess fees' sounds better.

transactions in specific ways you might find acceptable (e.g. for
layer-two protocols).

Wuille suggests two additions to what metadata is hashed: first, the
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could make it clearer that these would be optional sighash flags (ie the data you refer to below is only included if the flag is present). So change 'what metadata is hashed' to 'what metadata can be included in the hash' here, and 'the (transaction fee|scriptPubKey of the coins being spent) is included' to 'the (...) can be included' below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for changing this. I didn't include my rationale for wanting this to be more explicit. I wanted it to be clear that users wouldn't need to update all of their signing software if this change was introduced.

@harding harding force-pushed the 2018-11-27-newsletter branch from 292e8c6 to 5182591 Compare November 26, 2018 17:04
@harding
Copy link
Collaborator Author

harding commented Nov 26, 2018

Pushed update (force push as GitHub now provides diffs for those) that should resolve nits. Thanks for the review!

@practicalswift I'm happy to receive nits for extra whitespace at ends of lines and excessive newlines. I think either one or two spaces after full stops are fine even with a single author (and allowing them makes accepting contributions from multiple authors much easier).

@jnewbery
Copy link
Contributor

tested ACK 5182591. Thanks!

@moneyball
Copy link
Contributor

utACK. Made a minor typo fix.

@jnewbery jnewbery force-pushed the 2018-11-27-newsletter branch from dee04b9 to 7d774b7 Compare November 27, 2018 15:01
@jnewbery
Copy link
Contributor

Squashed @moneyball's commit and merged. Thanks all!

@jnewbery jnewbery merged commit 11f3f61 into bitcoinops:master Nov 27, 2018
jnewbery added a commit that referenced this pull request Nov 27, 2018
jnewbery added a commit to jnewbery/bitcoinops.github.io that referenced this pull request Aug 26, 2021
Newsletter bitcoinops#91 incorrectly referred to sighash_noinput as BIP116 instead of BIP118.
bitschmidty pushed a commit that referenced this pull request Jan 18, 2024
* newsletter283zh

* fix style

* Apply suggestions from code review

Co-authored-by: freeyao <[email protected]>

* Apply suggestions from code review

Co-authored-by: freeyao <[email protected]>

* fix link

---------

Co-authored-by: editor-Ajian <[email protected]>
Co-authored-by: Zhiwei(Jeffrey) Hu <[email protected]>
Co-authored-by: freeyao <[email protected]>
harding pushed a commit to harding/bitcoinops.github.io that referenced this pull request Sep 27, 2024
* newsletter283zh

* fix style

* Apply suggestions from code review

Co-authored-by: freeyao <[email protected]>

* Apply suggestions from code review

Co-authored-by: freeyao <[email protected]>

* fix link

---------

Co-authored-by: editor-Ajian <[email protected]>
Co-authored-by: Zhiwei(Jeffrey) Hu <[email protected]>
Co-authored-by: freeyao <[email protected]>
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.

4 participants