< gwillen>
promag: would you be interested in doing a full review of #14978
< gribble>
https://github.com/bitcoin/bitcoin/issues/14978 | Factor out PSBT utilities from RPCs for use in GUI code; related refactoring. by gwillen · Pull Request #14978 · bitcoin/bitcoin · GitHub
< gwillen>
(meshcollider mentioned your name but you weren't on at the time)
< gwillen>
he was suggesting a couple more reviews
< sipa>
gwillen: i will review soon
< promag>
will do tomorrow
< gwillen>
thanks sipa and promag!
< wumpus>
yes, std::string is binary safe, it is string encoding agnostic and doesn't enforce, like rust's strings, UTF-8. That said, using it for arbitrary binary data is bad because it communicates to developers "this is a string!".
< provoostenator>
Will review, but I'm going through the RNG stuff now :-)
< promag>
provoostenator: cool, stay with that :D
< wumpus>
15002 already has the tag backport
< wumpus>
it's impossible to avoid chaining PRs ,some work always depends on previous work that isn't merged yet
< promag>
wumpus: sure
< wumpus>
I agree it can be confusing, especially if the PR texts aren't clear on it, but in itself it's unavoidable; you can't expect people to keep working on something just because the precursor isn't merged yet
< wumpus>
eh *stop working on something*
< promag>
wumpus: I usually prefer that than big prs
< bitcoin-git>
bitcoin/master 059a3cf Wladimir J. van der Laan: contrib: Detailed reporting for http errors in github-merge...
< bitcoin-git>
bitcoin/master a4c5bbf Wladimir J. van der Laan: contrib: Add support for http[s] URLs in github-merge...
< bitcoin-git>
bitcoin/master f1bd219 Wladimir J. van der Laan: contrib: Allow use of github API authentication in github-merge...
< bitcoin-git>
[bitcoin] laanwj closed pull request #15165: contrib: Allow use of github API authentication in github-merge (master...2019_01_ghmerge) https://github.com/bitcoin/bitcoin/pull/15165
< fanquake>
+1 for 15170. Makes testing in random/temporary vms a bit easier
< wumpus>
pretty cool that the shellcheck author contributes himself
< fanquake>
yes. I wonder if he just stumbled on/was watching the project and decided to jump in, or someone reached out.
< bitcoin-git>
[bitcoin] promag opened pull request #15177: doc: Explain empty result of /rest/headers (master...2019-01-restheadersdoc) https://github.com/bitcoin/bitcoin/pull/15177
< promag>
wumpus: Marco reviewed #14958 yesterday
< gribble>
https://github.com/bitcoin/bitcoin/issues/14958 | qa: Remove race between conneting and shutdown on separate connections by promag · Pull Request #14958 · bitcoin/bitcoin · GitHub
< wumpus>
promag: ok
< wumpus>
promag: whoops in commit message "qa: Remove race between conneting and shutdown"
< bitcoin-git>
bitcoin/master 4412a59 João Barbosa: qa: Remove race between connecting and shutdown on separate connections
< bitcoin-git>
bitcoin/master 3ae3748 Wladimir J. van der Laan: Merge #14958: qa: Remove race between connecting and shutdown on separate connections...
< bitcoin-git>
[bitcoin] laanwj closed pull request #14958: qa: Remove race between connecting and shutdown on separate connections (master...2018-12-improve-shutdown-test) https://github.com/bitcoin/bitcoin/pull/14958
< bitcoin-git>
bitcoin/master 64ee943 Wladimir J. van der Laan: Merge #14409: utils and libraries: Make 'blocksdir' always net specific...
< bitcoin-git>
[bitcoin] laanwj closed pull request #14409: utils and libraries: Make 'blocksdir' always net specific (master...20181005-blockspath-always-netspecific) https://github.com/bitcoin/bitcoin/pull/14409
< bitcoin-git>
bitcoin/master b9dafe7 practicalswift: Fix remaining compiler warnings (MSVC). Move disabling of specific warnings from /nowarn to project file.
< bitcoin-git>
bitcoin/master 19c60ca Wladimir J. van der Laan: Merge #14151: windows: Fix remaining compiler warnings (MSVC)...
< dongcarl>
I don't know the intricacies of `static_cast` that well, and it seems like either there's something I'm not seeing or it was put there for no reason
< sipa>
i think empact is confused here
< dongcarl>
sipa: so no casting of any kind is needed there right? That would be cfields was also confused haha
< promag>
note that the old behavior `connman->Ban(pnode->addr, BanReasonNodeMisbehaving)` would go thru all nodes to disconnect (code that was moved to CConnman::DisconnectNode)
< promag>
in other words, iiuc, before SendRejectsAndCheckIfBanned could result in multiple disconnected nodes, now it only results in 1 one
< promag>
.. 1 node
< bitcoin-git>
[bitcoin] instagibbs opened pull request #15179: [trivial] Slight clarity boost to SEQUENCE_LOCKTIME_DISABLE_FLAG (master...rel_lock_lang) https://github.com/bitcoin/bitcoin/pull/15179
< promag>
dongcarl: or does this means that CSubNet(addr).Match() only returns true once?
< dongcarl>
promag: The version of `Ban` that takes in a `CNetAddr` constructs a single ip subnet (<ipv4>/32 or <ipv6>/128) `CSubNet` from that `CNetAddr`
< promag>
so only one node matches that?
< dongcarl>
unless 2 pnodes share the same addr, which I don't think can be the case but I'm double checking here
< promag>
dongcarl I think that very likely, if you have 2 nodes at home they share the same addr?
< dongcarl>
promag: You are right
< dongcarl>
promag: So what I think I'll do is for the `IsLocal` case I'll use fDisconnect, and use the pnode->addr in the other case
< promag>
you mean DisconnectNode(pnode->addr) in the else branch?
< dongcarl>
promag: right
< promag>
lgtm
< dongcarl>
promag: thanks for the help, lmk if you spot anything else!
< wumpus>
https://travis-ci.org/bitcoin/bitcoin/jobs/480544456 the only visible warning is the one about a missspelling in fuzzing.md, however that's an old warning and also spelling errors don't cause the test to fail
< meshcollider>
Is it the failing subtree checks above it?
< gkrizek>
I thought the same thing meshcollider but I can see those errors are still there in the last passing CI run
< gkrizek>
I think it's in contrib/verify-commits/verify-commits.py
< gkrizek>
If you look at the raw log from Travis there is this line at the end "Still running (1 of 50): contrib/verify-commits/verify-commits.py --clean-merge=2" doesn't show up in the UI