-
Notifications
You must be signed in to change notification settings - Fork 145
Newsletters: add #28 (2019-01-08) #99
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
Conversation
| currencies to create almost free [short-term options contracts][] by | ||
| delaying payment settlement. A [previous thread][cjp risk] by Corné | ||
| Plooy in May 2018 described the same thing. | ||
|
|
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.
Nit: Trailing spaces here :-)
| ## Action items | ||
|
|
||
| - **Upgrade to Bitcoin Core 0.17.1:** this new [minor][maintenance] | ||
| version released December 25th restores some previously-deprecated |
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.
Nit: Drop "-" in "previously-deprecated"?
| different currencies. Descriptions of notable code changes in popular | ||
| Bitcoin infrastructure projects are also provided. | ||
|
|
||
| ## Action items |
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.
Very minor nit: I don't know if we're strict we are with the Markdown header semantics, but in the case we are: since we're always starting with ## should we remove one # in the header level (from ## to #, etc.) to make it so that the first header is always a top level header?
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.
When using Jekyll, the title field in the YAML sets the the content of the rendered page's <title> and <h1> tags, so our top-level section are semantically correct for using ## to generate <h2>s.
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.
Makes perfect sense! Will not report this going forward :-)
c51b024 to
71e9fad
Compare
|
Some upstream developer decided to break the way we used Travis, so this PR now includes a commit with a temporary hack fixing it based on these instructions. Ultimately, everyone who generates local previews will have to upgrade our ruby versions, which is something we probably want to coordinate. |
jnewbery
left a comment
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.
Looks great. Thanks! A few minor nits inline.
Ultimately, everyone who generates local previews will have to upgrade our ruby versions, which is something we probably want to coordinate.
I think at the moment, it's just you, me and @moneyball who build locally, so we can go ahead and do this with minimum coordination.
| this may have happened to a large mining pool. | ||
|
|
||
| - [Bitcoin Core #14336][] changes the system call (syscall) Bitcoin Core | ||
| uses to handle network connections and other resources on platforms |
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 think this description should focus on file descriptors, rather than talk about network connections. This PR was prompted by a user reporting they were hitting the file descriptor limit. See discussion in #bitcoin-core-dev irc on 2018-07-31 and 2018-08-01:
218 2018-07-31T19:41:23 <ossifrage> it looks like bitcoin-qt is using up 800 FDs for open files (chainstate, txindex, etc...)
26 2018-08-01T00:43:02 <gmaxwell> ossifrage: why did you say above that ldb was using lots of FDs?
27 2018-08-01T00:43:25 <ossifrage> That was from the output of lsof and some counting
28 2018-08-01T00:44:21 <BlueMatt> that seems...surprising, given its supposed to do mmap and then close the fd
29 2018-08-01T00:44:25 <sipa> i have a number of chainstate files open by bitcoind as well - most are mmaped, but not all
30 2018-08-01T00:44:27 <BlueMatt> so it may still show up in lsof but not use an fd?
31 2018-08-01T00:44:46 <ossifrage> I was counting file descriptors not maps
32 2018-08-01T00:45:28 <sipa> i have 30 chainstate files open with an FD, and 999 with mmap
33 2018-08-01T00:45:29 <ossifrage> The node has been up for >30 days if that makes any difference
34 2018-08-01T00:45:46 <gmaxwell> come on, why can't we just take the not-many-line-change to use poll? I know libevent future ra ra ra... but we have held off this simple fix for years. :(
...
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.
That user only encountered those problems because they were running with more than the default maximum number of connections:
229 2018-07-31T19:58:47 <sipa> how many connwctions do you have?
230 2018-07-31T20:01:26 <ossifrage> sipa, 160
IIUC, the lack of file descriptors never caused problems with the database---the user-visible problem was that sometimes the user couldn't previously get to their configured maximum number of connections, as we described in Newsletter 8:
Bitcoin Core #13925: increases the maximum number of file descriptors Bitcoin Core’s internal database can use, which can allow more file descriptors to be used for network connections. If you’ve modified Bitcoin Core to accept more than 117 incoming connections, you may see an additional increase in the number of connections after upgrading past this merge. (Note: we don’t recommend increasing the default unless you have a special need.)
We also previously discussed some technical aspects of this issue: #58 (comment)
In short, I think the description in the newsletter now describes the user-visible changes, which are what I think is most important to our readers, rather than going into the technical details (which are still available to anyone who clicks the link to the PR).
I don't think I'll be around to see your reply to this issue before publication time, so feel free to (1) modify the text as you think is appropriate or (2) drop the bullet point from this newsletter and we can discuss it and optionally put it in next week's newsletter.
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 still haven't fully wrapped my head around the motivation for this change (and the best way to describe it) so I'm going to drop this bullet point from this week's newsletter so we can discuss further and include next week.
|
|
||
| - [C-Lightning #2172][] allows `lightningd` to be shutdown normally even | ||
| if it's operating as the primary process (PID 1), which can be useful | ||
| in Docker containers. |
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.
Optionally, you could mention that this is how c-lightning is run by BTCPayServer.
| plugin][cl helloworld.py] have been updated for these handlers. | ||
|
|
||
| - [LND #2374][] increases the maximum size of the gRPC messages the | ||
| `lncli` tool will accept from 4 MB to 50 MB. This fixes a problem |
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.
minor nit: I found the "will accept from..." tripped me up on my first parse - the from attached itself to accept instead of increases. You could consider parenthesizing from 4 MB to 50 MB (although I'll admit I'm often too liberal with parentheses)
| exceed even this new maximum, so developers are planning to revamp the | ||
| relevant RPCs to handle this situation. | ||
|
|
||
| - [LND #2354][] adds a new `invoicestate` field and deprecates the former |
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 think 'removes' is better than 'deprecates'. Deprecates suggests to me that the field is still there but will be removed in future, but in fact PR 2354 removes the field.
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.
Are you sure? I don't run lnd, so I can't quickly check, but I wrote my description based on how I guessed it worked from the patch:
diff --git a/lnrpc/rpc.proto b/lnrpc/rpc.proto
index 8794c1b4..10e53179 100644
--- a/lnrpc/rpc.proto
+++ b/lnrpc/rpc.proto
@@ -1699,7 +1699,7 @@ message Invoice {
int64 value = 5 [json_name = "value"];
/// Whether this invoice has been fulfilled
- bool settled = 6 [json_name = "settled"];
+ bool settled = 6 [json_name = "settled", deprecated = true];I don't know enough about gRPC in general or lnd in particular to know whether this removed the field or just tags it as deprecated. However, I would consider it poor form on their part to remove a field without deprecating it first.
I rewrote the opening of this bullet so that it's consistent with either of our interpretations, but feel free to edit as you think appropriate.
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.
You're right and I'm wrong. This is deprecated and not removed. The settled field is set from the ContractTerm.State field here: https://github.com/lightningnetwork/lnd/pull/2354/files#diff-01ada5680aaa18fc79cbf78b2d55a37bR3306
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've reverted your change back to '...and deprecates the former'
| [discussed][rm codesep] is whether the `OP_CODESEPARATOR` opcode | ||
| should be disabled in a script update that supports MAST (e.g. via | ||
| Taproot). That opcode is not in common use, but if you plan to use | ||
| it in future Script versions, you should comment on the thread. |
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.
Should this be in the Action Items section as well?
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 think it's ok to leave this a bit buried. We do want people to comment sooner than later, but I think it's ok if nobody comments until there's been an actual formal proposal of a soft fork with a new Script version that doesn't include this opcode. Besides, my guess is that nobody is using this opcode.
|
aside from my one comment/question, utACK |
|
For the Travis workaround, I suggest we use it for this newsletter and the next one ( |
We'll discuss this further and include in next week's newsletter.
292f22d to
1986b61
Compare
* Newsletter-291: translate into Chinese * Apply suggestions from code review Co-authored-by: editor-Ajian <34365188+editor-Ajian@users.noreply.github.com> * "trigger" -> “扳机” --------- Co-authored-by: editor-Ajian <34365188+editor-Ajian@users.noreply.github.com>
* Newsletter-291: translate into Chinese * Apply suggestions from code review Co-authored-by: editor-Ajian <34365188+editor-Ajian@users.noreply.github.com> * "trigger" -> “扳机” --------- Co-authored-by: editor-Ajian <34365188+editor-Ajian@users.noreply.github.com>
Happy 2019. :-)