< fanquake>
sipa / wumpus could you block Stivovo from GH. They are spamming nonsense in multiple threads.
< bitcoin-git>
[bitcoin] scravy closed pull request #15715: Better support for mainframes and EBCDIC users in general (master...cater-mainframes-and-ebcdic-users) https://github.com/bitcoin/bitcoin/pull/15715
< wumpus>
fanquake: yes, probably for the best
< fanquake>
missed some discussion last week, but must be close to an rc3 post #15691.
< bitcoin-git>
[bitcoin] practicalswift opened pull request #15721: validation: Check absence of locks at compile-time (LOCKS_EXCLUDED) in addition to the current run-time checking (AssertLockNotHeld) (master...negative-locking-annotations) https://github.com/bitcoin/bitcoin/pull/15721
< wumpus>
jamesob: 120 as max line length suggestion sounds fine to me
< wumpus>
jamesob: though it will probably run into tons of discussion, which is why no one never did that
< jamesob>
I think "causes the need to scroll horizontally on github" is a pretty good standard for too long, so I'll just measure whatever that amounts to in terms of column length
< wumpus>
depends on your screen width and font size
< emilr>
even 120 seems a bit too much, it's not about screen or font size, our brains are used with reading books 80c width lenght for hundreds of years
< dongcarl>
Looking at #15717, not too familiar with how licenses work but it seems that libnatpmp has this license: https://pastebin.com/a6umcv4s
< dongcarl>
wumpus: I guess the question is if we should subtree it or use it as a dependency? Right now it seems like the PR author wants to subtree it
< sipa>
it's not a subtree
< sipa>
he's copying the code with some modifications
< wumpus>
it's a minimal amount of code, I think it would make sense to subtree, though I'm sure this is going to give tons of discussion
< sipa>
the upstream code hasn't been touched since 2011
< wumpus>
whoa
< wumpus>
that is a big red flag
< wumpus>
this effectively means we're going to have to maintain it ourselves
< wumpus>
if someone commits to maintaining it it's fine with me, I'm just not going to do it
< sipa>
fair
< sipa>
regarding licence issues... i think we just need to list it in asset-attributions
< sipa>
but ianal
< sipa>
i have no idea about the complexity of the protocol... if it's simple enough (or at least the parts we need are simple enough), reimplementing just that part may be preferable
< wumpus>
I'm not sure...
< sipa>
miniupnp has a history of vulnerabilities... is this written by the same authors?
< wumpus>
yes
< wumpus>
though it's easier to get right, at least there's no xml generation/parsing in here
< wumpus>
though it's still quite a heap of C code
< wumpus>
in any case that can be improved later, I guess
< dongcarl>
Reading thru the libnatpmp repo... It seems extremely simple...
< wumpus>
it's great that someone is working on this
< dongcarl>
I think we just need natpmp.{c,h} and getgateway.{c,h}
< dongcarl>
Can someone explain to me what the difference between subtree-ing and just copying code is?
< jamesob>
dongcarl: pulling in from upstream is way better with a subtree
< wumpus>
subtreeing sets up git to easily update to newer versions of the tree, also it allows preserving commits (though we don't do this)
< sipa>
subtree also only makes sense if upstream is a git repo
< wumpus>
yes
< wumpus>
I think mentioning the files in assets-attribution makes sense, though we've never before done this for code
< wumpus>
this would nto be acceptable for a completely incompatible license such as (L)GPL, but MIT/BSD is close enough
< dongcarl>
I see, so we should copy in the files then. Something like `src/libnatpmp`?
< wumpus>
it doesn't 'infect' the project, nor the licensing of the binary
< wumpus>
dongcarl: that's what they do right?
< dongcarl>
Ah. Right.
< luke-jr>
something like libnatpmp should just be a dependency, not copied or subtree'd at all -.-
< luke-jr>
jamesob: best not to review in github anyway
< jamesob>
luke-jr: agree but inevitably you end up reading stuff in GH since we leave comments there
< gmaxwell>
I don't think natpmp is worth taking a sketchy library dependency, it's only supported by a relatively small number of devices. It sounded attractive when I expected its implementation to be two structs and <50 loc.
< luke-jr>
gmaxwell: is that certainly not possible?
< gmaxwell>
no one seems interested in doing it.
< gmaxwell>
esp with the bad history of miniupnpc... taking /another/ dependency on code from there just seems kinda foolish.
< wumpus>
especially one that hasn't been updated since 2013
< wumpus>
either we merge it in and someone picks up maintenance of it, or it's dead in the water
< gmaxwell>
that in and of itself isn't /necessarily/ a concern, it's a really simple protocol. Unlike C++ in C the language isn't shifting out from under you. :P
< wumpus>
looks like the person maintaining it already cut it down a lot, btw
< gmaxwell>
but its not not a concern either.
< wumpus>
to only contain the parts that are needed for bitcoind and nothing else
< wumpus>
s/maintaining/submitting
< gmaxwell>
thats good there was a lot of stuff we didn't need in there before for sure.
< gmaxwell>
(pmp is a small protocol and we only need a small subset of it)
< luke-jr>
it looks like 2015 to me?
< gmaxwell>
The ongoing sybil/eclipse attacks on the network highlight the need to get more ordinary users esp ones not on vpses listening.
< sipa>
it's still 1100 lines of imported code or so
< wumpus>
I honestly don't think we're ever going to agree on this, I just want to encourage the PR author to continue this work tbh
< wumpus>
I could see this being abandoned again because everyone wants something else
< gmaxwell>
I don't want to stand in the way of it. If we do go the library route I'll still go review the libraries code, even though I'm wary of it considering the source (and the surprisingly large size given how little we need).
< gmaxwell>
(My (maybe bitrotted) understanding is that this protocol requires we send and recieve a single udp packet with a fixed layout struct. The only moderate complexity comes in via the fact that we need to get the default gateway IP, which needs some OS specific code)
< sipa>
it seems finding the gateway is a significant portion of the code
< wumpus>
well, currently the code is being imported, I think it should stay like that
< wumpus>
sipa: that might well be the most difficult part
< wumpus>
also, testing
< wumpus>
this seems difficult to test without building a whole multi-VM network setup
< wumpus>
though, everything considered, I don't think we test upnp functionality at all at the moment
< gmaxwell>
I'd rather have it without tests than not have it.
< wumpus>
(nor ever did)
< wumpus>
I mean, if people with a router that support it test it that'd be something
< dongcarl>
I'd be happy to test it with a natpmp daemon on my gateway
< gwillen>
(or does anybody else want to take a final look?)
< conman>
I'm trying to find the code that prioritises block inclusion according to GetModifiedFee() in CreateNewBlock() and I can't see it. I've tried using prioritisetransaction and confirmed it's getting a fee increase way above any other transactions but not included in getblocktemplate
< conman>
has this functionality been confirmed working in recent releases?
< conman>
I've not tried it in a couple of years
< gmaxwell>
conman: there are tests for it, it appears to be working to me.
< gmaxwell>
conman: its tested by mining_prioritisetransaction, mempool_persist, and mempool_packages
< gmaxwell>
conman: is the transaction not being included for some other reason? (invalid, non-standard, too large?)
< conman>
it's definitely in the mempool, I've tried editing the code to make sure it's included by manually setting bypass_limits in sendrawtransaction and that makes no difference, and it's a small regular sized tx
< conman>
I'm baffled as to why it won't show up in a getblocktemplate though
< gmaxwell>
conman: lol
< conman>
:\
< conman>
yulol
< gmaxwell>
conman: is your problem that it uses segwit (or has unconfirmed parents that do) and you're not calling GBT with the segwit flag?
< conman>
nah
< gmaxwell>
darn
< conman>
definitely using segwit
< gmaxwell>
Are its parents confirmed?
< conman>
yeah
< gmaxwell>
What code are you running?
< conman>
0.17.1 vanilla
< conman>
(without the hack above I tried)
< gmaxwell>
You're using the fee_delta argument to prioritisetransaction? not the second argument which is now a dummy?
< conman>
it won't let you set anything but 0 there anyway
< gmaxwell>
I dunno what to say, I can see it reordering transactions for me. and the test looks reasonable (it's actually testing that it gets txn into the block that otherwise wouldn't)
< * conman>
scratches head
< gmaxwell>
sdaftuar: ^ any thoughts?
< gmaxwell>
conman: does getmempoolentry look sensible?
< conman>
sec
< conman>
yes
< conman>
shows modifiedfee too
< * gmaxwell>
puts on his good tech support hat
< gmaxwell>
conman: are you querying the right node?
< gmaxwell>
can you look at your template, grabe a txid from somewhere in the middle, and see that it looks worse in getmempoolentry?
< conman>
heh
< gmaxwell>
(sorry to ask a dumb question, but too often I've seen those solve problems. :) )
< conman>
I assume you mean modified is lower, yes
< conman>
even the top entry in the block template has a lower modified fee than this
< conman>
should the wtxid be different to the txid in the mempoolentry ?
< gmaxwell>
for non-segwit txn wtxid and txid are the same, for non-sw they're different.
< gmaxwell>
er for sw they're different
< gmaxwell>
sorry, distratcted. :)
< conman>
roger
< conman>
well it's a segwit txn
< conman>
that's the only thing that's different from when I did this 2 years ago
< conman>
they were regular txns
< conman>
can't imagine that's the reason but that's the only thing I've changed
< * gmaxwell>
looks to see if the test use segwit txn, maybe its broken for those!
< conman>
[08:32] <gmaxwell> conman: its tested by mining_prioritisetransaction, mempool_persist, and mempool_packages
< conman>
I don't see a mining_prioritisetransaction anywhere
< conman>
what function are you actually referring to?
< gmaxwell>
the tests are in bitcoin/test/functional/mining_prioritisetransaction.py
< conman>
oh I was looking in src/
< gmaxwell>
you can run it by running ./test_runner.py mining_prioritisetransaction.py it runs fine on a system already running a node.
< gmaxwell>
looks to me like the test will use segwit (it'll use whatever address type the node returns by default)
< gmaxwell>
But I could be misreading the test.
< conman>
I see a fixed txid in the code
< gmaxwell>
thats just testing an invalid value
< * conman>
scratches head
< conman>
I'll build and try the test
< gmaxwell>
thanks.
< conman>
says it passes, but I can't actually give it a transaction of my choice to see if that's okay
< conman>
anyway got to run damnit, bbl to do some more debugging... really can't see why it won't show up in the template
< conman>
thanks so far
< gmaxwell>
are you perhaps confusing wtxid and txid in the template or something and it's there? (sorry last ditch guess)
< achow101>
gwillen: make a wallet without private keys. import something using importmulti and set 'keypool': true
< gwillen>
achow101: so I've been fiddling with it
< gwillen>
it seems like the magic I was initially missing was that "internal": true is mandatory with "keypool": true, or it has no effect
< gwillen>
which makes sense in retrospect
< gwillen>
also, the address has to be unused
< gwillen>
or it will go into the keypool but not be considered for use as a change address
< achow101>
internal: true only makes it a change address
< achow101>
it isn't mandatory
< conman>
darn it confirmed, now I can't try it again
< gwillen>
... you know I was so focused on change addresses I actually forgot that they keypool is used for other stuff
< gwillen>
like, receiving addresses
< gmaxwell>
conman: try some other txn
< conman>
yeah when I get a chance later
< conman>
oh you mean someone else's transaction
< conman>
but yeah when I have time
< conman>
I do think there's something actually wrong
< gmaxwell>
I'd believe it, the interface isn't used much anymore AFAIK, and so maybe there is some issue that the test happens to not trigger or maybe in some particular config.