< moneyball>
does anyone know how to find all open PRs that modify a particular file (ie validation.cpp)?
< pinheadmz>
github you can view history of a single file
< pinheadmz>
or check the "blame"
< moneyball>
pinheadmz thanks but that only shows history/merged PRs and not open PRs, right?
< booyah_>
why bitcoin uses ECDSA on secp256k1 instead of Ed25519, any arguments for/against?
< sipa>
booyah_: ask satoshi
< sipa>
as for why it hasn't changed since: they're similar in performance, changing is hard, ed25519's cofactors are annoying to build more complex constructions on
< sipa>
(and probably more on topic in #bitcoin-wizards)
< booyah_>
sipa: ok then implementation question, secp256k1 support is home-made and probably will remain so, as opposed to eg using a general library like nacl/sodium?
< sipa>
i don't see good reasons to change it
< MarcoFalke>
moneyball: could be an idea for bitcoinacks by pierre_rochard
< sipa>
booyah_: it would be slower, introduce new review burden, and any change of that kind is risky
< gmaxwell>
ed25519 is unambigiously less secure too, though not in an amount that would matter.
< sipa>
by about 1-1.5 bits or so, yeah
< gmaxwell>
and re cofactor, several cryptocurrencies have had serious vulnerabilities due to using ed25519.
< gmaxwell>
E.g. monero/cryptonote/etc. had an unbounded inflation bug due to cofactor handling.
< gmaxwell>
basically, the ed25519 stuff in general is okay, it makes its own tradeoffs, but it extremely overhyped with a lot of misleading marketing.
< booyah_>
gmaxwell: which attack works on ed25519 but not on secp?
< sipa>
booyah_: i would hope none!
< sipa>
it's just harder to get complex protocols on top right
< gmaxwell>
booyah_: cofactor attacks, as I mentioned.
< sipa>
ed25519 is a digital signature system, and when used as just that - it's perfectly fine
< sipa>
but something like bip32 is already somewhat harder to do for ed25519
< gmaxwell>
and ... has been done wrong multiple times.
< echeveria>
people generally misunderstand that ed25519 is not a single show signature, due to the marketing.
< echeveria>
this has resulted in vulnerabilities as well. it’s deterministic as secp256k1 is, with rfc6979, but this is not a method of making signatures single show.
< sipa>
i think you mean "unique" instead of "single show"
< booyah>
what is single show signature?
< sipa>
a signature that leaks the private key as soon as you sign two different messages with the same key
< echeveria>
sipa: yeah, wrong terminology I think.
< sipa>
a unique signature is one where every message/pubkey pair has exactly one valid signature
< booyah>
hmm?? ed25519 (nor secp256k1) are not single-show right? lamport is
< sipa>
correct
< sipa>
which is why i think echeveria meant unique signature, not single show
< echeveria>
booyah: right. a well known altcoins uses this “property” of ed25519 to protect against stake grinding attacks. their misunderstanding of the description of ed25519 make them believe the output was unique, where the text suggested the outputs were deterministic.
< sipa>
(neither ed25519 nor secp256k1 are unique, but due to the deterministic signing constructing in ed25519 people sometimes think it is - but it's no different than using rfc6979 with secp256k1)
< echeveria>
yes. the text just meant the reference library was 6979 by default. bizarrely a lot of ecdsa libraries still aren’t.
< booyah>
what would be signature being unique, and being deterministic, in this context?
< booyah>
deterministic = for same cleartext, and same privkey, it produces identical signature?
< sipa>
deterministic is a property of a signing implementation, not a property of the signature scheme
< sipa>
unique means that the only possible implementation is deterministic
< sipa>
BLS for example is unique
< echeveria>
in the context of the signer, they choose their nonce based on the H(message + privkey) rather than choosing random bytes. it is impossible to prove this happened to an untrusted observer.
< sipa>
wellll.... (/me wonders off into ZKP land)
< provoostenator>
luke-jr: I'm still open to either cpp-subprocess or boost::process or something else; need to study them more. It shouldn't be too hard to swap them in my PR.
< gwillen>
the wallet unloading logic is uh, intricate
< gwillen>
the thing #15195 is doing seems safe to me, but I don't know how long a wallet can take to unload; if it's a long time it could definitely be confusing.
< provoostenator>
luke-jr: why would absense of a package be reason not to use something? I would say the biggest criterium is quality.
< provoostenator>
But if boost:process already works with Windows I will probably switch to that anyway.
< gmaxwell>
welp jgarzik on that pr. pressing close. best of luck to you.
< promag>
?
< wumpus>
the biggest criterion is "please don't introduce any security bugs"
< luke-jr>
biggest criterion != only criterion
< promag>
another one: use stable dependencies?
< gmaxwell>
more like avoid dependencies entirely, unless they provide a lot of value.
< luke-jr>
dependencies > NIH reinvention
< provoostenator>
I can't believe we even need dependencies for something as basic as running a command and getting the result.
< gmaxwell>
(with 'a lot' depending on how universally used they are.)
< gmaxwell>
provoostenator: yeah
< provoostenator>
You'd think 40+ years of C development would have solved that...
< luke-jr>
well, commands are inherently external to C
< luke-jr>
iirc
< wumpus>
provoostenator: +100
< wumpus>
so rust implementation of bitcoin core when?
< luke-jr>
Rust compiler that can be safely bootstrapped when? :/
< luke-jr>
I guess the problem with popen is Windows
< rafalcpp>
provoostenator: well you can just run command easily, but boost process just handles more options in it
< gmaxwell>
provoostenator: it's trivial to do in standard C on unix, ... the thing I'd expect a dependency to handle is windows. But it sounds like this one doesn't do windows?
< luke-jr>
boost::process is newer than our current minimum version it seems; wonder if we can just bump it
< wumpus>
gmaxwell: was about to say that
< wumpus>
so can boost::process do the required thing on windows?
< wumpus>
I kind of hate to introduce a new boost dependency but I think I hate the alternative even more
< gmaxwell>
Certantly if we do need to add a dep for this (E.g. to get windows support) I'd rather take boost than some obscure package.
< wumpus>
right
< wumpus>
especially as boost tends to be picked up in future c++
< gmaxwell>
Also there is at least some chance that the boost thing shows up in C++24 or whatever.
< provoostenator>
I haven't checked yet. I spent most of yesterday dealing with make exploding over the order in which UniValue and Util were loaded (since this methods a UniValue)
< rafalcpp>
what do you require of bp?
< luke-jr>
boost::process is new in 1.64.0; Debian is at 1.62 :/
< provoostenator>
rafacpp: run a command, process its std::out
< rafalcpp>
wait I think I did that in debian stable
< provoostenator>
(and parse that if it's valid JSON, otherwise fail)
< luke-jr>
RHEL is at 1.53 x.x
< wumpus>
eh that JSON parsing we can handle ourselves
< wumpus>
the depenency just needs to be able to provide stdin data and give us stdout data
< provoostenator>
wumpus: correct, UniValue does that
< wumpus>
like python's subprocess has been able to do, what, 20 years?
< provoostenator>
Yes
< provoostenator>
One even hack would be to use the existing code for a calling a command, have it pipe the result to a file and then poll the filesystem for it. But I'd rather not.
< provoostenator>
*evil
< wumpus>
oh no
< wumpus>
we're going do this properly or not at all
< provoostenator>
Indeed. And it shouldn't be that hard.
< provoostenator>
The test suite uses a hack along these lines to the notification code by the way.
< wumpus>
and maybe that can be replaced too when have a way to do it better
< promag>
one important thing, that we don't handle right, is thread safety - boost::process isn't thread safe also
< wumpus>
promag: what???
< luke-jr>
^
< gmaxwell>
what do you mean by "thread safe" in this context?
< wumpus>
making new processes should be about the most thread safe thing ever
< promag>
we launch a thread that will call system(cmd)
< promag>
this is not thread safe
< wumpus>
how is it not?
< promag>
iiuc it can mess file descriptors of one process to other if concurrent systems are called?
< gmaxwell>
system() is prefertly multithread safe.
< rafalcpp>
Stroustrup *invents C++*
< rafalcpp>
40 years passes. hEy guYs mayBE we shOUld have clAssEs to do ProcESSes and shit
< gmaxwell>
(at least the C function is)
< luke-jr>
I'm shocked there isn't an obviously good solution to this
< rafalcpp>
maybe reimplement mininmal version of boost process and include it
< rafalcpp>
or build own boost process on our own by copying the code (assuming it doesn't have other boost deps goind too deep)
< promag>
gmaxwell: I read different opinions, I don't have deep knowledge here
< wumpus>
rafalcpp: similarly I'm surprised there is no standardized networking between platforms yet, certainly the async stuff, I mean, TCP/IP is not exactly an experimental toy anymore, decennia later
< wumpus>
promag: this isn't about opinions, either that call is thread-safe or it is not, I can't find anythingabout thread safety on the manpage at least
< luke-jr>
eh? my system(3) manpage says it's MT-safe
< wumpus>
it has *tons* of problems, for example escaping security issues, but I don't think thread safety s one of them
< wumpus>
system() │ Thread safety │ MT-Safe
< wumpus>
oh you're right luke-jr
< luke-jr>
of course, this is just glibc
< wumpus>
well yes but ...
< promag>
wumpus: setenv is not thread safe :(
< wumpus>
"fix your libc this is not 1995 anymore"
< gmaxwell>
The SO answer also supports it being safe in general-- points out its safe on linux, the only counter example it gives is solaris but there the only issue is changes to signal handlers.
< wumpus>
promag: I know
< wumpus>
promag: that's why you do it *after* forking
< luke-jr>
Windows has no fork :P
< wumpus>
windows is completely differnt
< gmaxwell>
essentially, when you fork, you lose your other threads, so you can't do anything that will interact with them.
< wumpus>
in *any case* you can pass a new environment to the spawn call
< gmaxwell>
Calling plain libc functions and syscalls is fine, however.
< wumpus>
both on windows and linux
< wumpus>
but yeah not with system() which is kind of limited
< promag>
wumpus: how=
< wumpus>
sometimes I think the only real use of system() system call left is to spawn shells in exploits
< gmaxwell>
lol
< gmaxwell>
"Highly optimized for return only programming."
< wumpus>
:-)
< promag>
btw, could we just support %w on non-win?
< promag>
..for now.. X)
< rafalcpp>
hould we write own minimal process library?
< achow101>
why not boost::process?
< rafalcpp>
achow101: too new
< promag>
is there anyone using *notify commands on windows?
< luke-jr>
if it does what we need, maybe we can just embed a copy of boost::process used when the OS lacks a new enough version?
< gmaxwell>
if boost process being too new is the only issue with it and its otherwise attractive and handles window, we could also potentially just copy it temporarily into the codebase.
< wumpus>
promag: for example `execve` has a envp argument, you should really never have to use setenv()
< luke-jr>
it's not ideal, but better than the alternatives I guess
< rafalcpp>
gmaxwell: might be too dependant on other boost
< gmaxwell>
(though in the past I've found that its hard to copy out just parts of boost, because boot itself is a full on boost koolaid drinker)
< luke-jr>
rafalcpp: might be, but probably not
< gmaxwell>
^
< * rafalcpp>
bjam's it
< luke-jr>
rafalcpp: prior to inclusion in boost, it was separate in fact
< luke-jr>
gmaxwell: even if it's on boost koolaid, we use boost anyway, so that shouldn't be a big issue
< wumpus>
luke-jr: unless it uses new boost features
< luke-jr>
true
< wumpus>
(which could be expected if it's new code)
< luke-jr>
but then we can just embed a copy of an older boost::process before it got merged
< luke-jr>
assuming it's compatible
< rafalcpp>
luke-jr: soo yeah... let's just find the base library for process, review it based on boost::process experience and use that?
< rafalcpp>
I'm interested, because on project done with my friend I have similar problem, so we also could use/help with such process lib if we can
< achow101>
also we probably won't need to have to be able to send things over stdin. hwi is designed to have everything entered in a single command
< wumpus>
I think it's pointless to re-invent boost::process
< luke-jr>
achow101: as we're learning with walletnotify, a single command is a mess
< luke-jr>
so I think stdin would be preferred?
< wumpus>
like "this already exists in a newer verison of boost but we need to write our own anyway *just* for other distributions, a problem that will automatically go away with waiting"
< wumpus>
I like waiting and doing nothing more
< promag>
what does pyhton subprocess uses in windows?
< luke-jr>
wumpus: just a limited copy used when boost is old, IMO
< rafalcpp>
" long attempt to get a boost.process library, which is going on since 2006. " wow.
< achow101>
luke-jr: it was the only way I could have every device have a consistent interface
< luke-jr>
promag: Python is a giant blob
< achow101>
otherwise some would require stdin for some thigns and others wouldn't
< luke-jr>
achow101: ?
< wumpus>
it's absurd that something simple like this is giving so much problems
< wumpus>
bring back the 80's please
< luke-jr>
anything you can do with a command line, you can do with stdin..
< gmaxwell>
achow101: commandline lengths are limited, unfortunately. (also... not private)
< achow101>
luke-jr: trezor asks for a scrambled pin. no other device does that
< luke-jr>
achow101: so?
< achow101>
you could enter it over stdin, but then you have to special case for the trezor to know to send it something over stdin
< luke-jr>
achow101: I'm saying do EVERYTHING over stdin
< gmaxwell>
certantly it would be nice to not need stdin, but I believe the maximum size of a transaction is above the commandline limit even on bog standard linux.
< luke-jr>
gmaxwell: on Windows, the command line has no standard quoting
< gmaxwell>
I'd always assumed this stuff would use stdin exclusively.
< rafalcpp>
so many ways to snoop on cmd args from other local users
< luke-jr>
gmaxwell: the %w promag keeps bringing up is a mess because apparently we want to escape the wallet name
< rafalcpp>
how about creating a tmp file (chmod for privacy), and pass it's name in arg?
< luke-jr>
rafalcpp: …why?
< wumpus>
nOoooOooo *screams*
< gmaxwell>
rafalcpp: there be dragons, decades of security disasters from that..
< rafalcpp>
hmm to hide the text from ps aux, and at same time not require stdin/out process library...? what security disasters, when done right?
< wumpus>
please do the sane thing and have some protocol over stdin/stdout, don't try to do hacks with command lines, or temporary files, or env variables, or ...
< gmaxwell>
I think insecure tmp files is probably neck and neck with buffer overflows for the origin of the most CVEs.
< wumpus>
we really can't afford stupid mistakes here
< wumpus>
gmaxwell: as well as shell escaping issues
< wumpus>
this is like bug paradise
< rafalcpp>
just chmod it? and mktemp exists for atomic creation of unique file?
< wumpus>
*cries*
< gmaxwell>
rafalcpp: it's an ugly rathole. Stdin is a straight forward, clean solution.
< gmaxwell>
It would be a 4-line no brainer except for windows.
< rafalcpp>
btw, how about IPC or RPC?
< wumpus>
chmod also isn't available on windows, or at least, very different
< gmaxwell>
but it sounds like the boost thing handles windows fine.
< gmaxwell>
rafalcpp: also a security disaster.
< wumpus>
IPC or RPC is *more difficult* than stdin and stdout handling
< wumpus>
it's also highly platform specific
< wumpus>
especially if you want only the child process to be able to access it
< gmaxwell>
(consider the recently published rpc cve in bitcoin that we basically cannot fix...)
< rafalcpp>
you're right, stdin is most secure
< promag>
what if we assign a unique id to the wallet - which is passed to cmd - and then the cmd can use that to know more details?
< wumpus>
not more hacks
< wumpus>
stop this
< promag>
right, thanks
< rafalcpp>
boost process seems to be getting bugfixes as recent as a month ago. I guess best to use that version (insted of going back to old version before move into part of boost)
< gmaxwell>
I know, we could embed a SMTP mail user agent, and send email to the other process.
< gmaxwell>
oh oh oh, I know. I know. We could use ... A BLOCKCHAIN.
< luke-jr>
lol
< wumpus>
gmaxwell: sometimes I feel we've reached the point where it's trivial to spin up a complete host environmet including mail server and website, but impossible to do even basic things like spin up a process and interface with it
< gmaxwell>
first we'll issue utility tokens for interprocess communications....
< promag>
smart processes? lgtm
< wumpus>
yes dunno build a IoT toilet plunger that changes color on a new transaction for notification
< provoostenator>
gmaxwell: there is no maximum length for PSBT either... and if you'r spending legacy stuff to a very careful (and powerful) device, stdin might indeed be very useful.
< achow101>
we can add an option to hwi to send all arguments over stdin. right now it's completely command line based
< wumpus>
soo
< wumpus>
my proposal would be: first build this with boost::process
< gmaxwell>
achow101: presumably you lack tests with large PSBTs right now then. :)
< wumpus>
if, by the time this gets merged, the availability of that is still a problem, we can put in the effort to make something ourselves or adapt something
< wumpus>
but please let's not take design shortcuts because of short-term easy availability of libraries, you'll hate yourself for that later, promised
< achow101>
gmaxwell: the tests currently don't use the cli directly (as in use subprocess). so late psbts probably wouldn't trigger a problem in current tests anyways
< luke-jr>
wumpus: good point, this can be an optional feature for newer boost versions for the initial PR, and then address that separately
< wumpus>
luke-jr: +1
< gmaxwell>
wumpus: or put in efforts to extract boost-process from boost and package it as a compat.
< wumpus>
gmaxwell: right
< wumpus>
promag: as for your case, I don't think there's any option on windows, if you want to pass the wallet name on the system() command line, than to use an encoding like base64. It's inconvenient for the receiving process, but it's better than nothing (I guess) and then again it's their fault for using a broken OS :)
< provoostenator>
achow101: the tests in #14912 (which is still based on popen) send a real (regtest) PSBT to a real command.
< provoostenator>
But a rather small one, so I 'd have have to generate a bigger one in the functional test suite.
< provoostenator>
I created a seperate PR to figure out how to properly call an external command and process the result because I had a feeling it would otherwise overwhelm the main PR :-)
< wumpus>
promag: I know I was originally against the idea of using an encoding but I had never expected to be like this
< wumpus>
passing the information to the notify script through stdin would be a future option too I guess...
< provoostenator>
As for v0.18 I'd be quite contend if #14021 and #14075 merged, so the HWI Python scripts can be used without compiling a custom branch.
< gribble>
https://github.com/bitcoin/bitcoin/issues/14075 | Import watch only pubkeys to the keypool if private keys are disabled by achow101 · Pull Request #14075 · bitcoin/bitcoin · GitHub
< bitcoin-git>
bitcoin/master cbe7efe Wladimir J. van der Laan: Merge #15389: Remove unnecessary const_cast
< wumpus>
provoostenator: yep; I see those are both already on the milestone
< wumpus>
oh #14075 isn't
< gribble>
https://github.com/bitcoin/bitcoin/issues/14075 | Import watch only pubkeys to the keypool if private keys are disabled by achow101 · Pull Request #14075 · bitcoin/bitcoin · GitHub
< provoostenator>
Afaik it's only a problem when there is a ginormous reorg. A fix also needs to be backported, since this issue has been around for some time.
< gmaxwell>
it also screws up the usability of invalidateblock/reconsider block for testing.
< wumpus>
yes, it's a bad issue, there's just not that much time left
< wumpus>
I mean, unless someone opens a PR for it in the next few days, there's very little chance for a fix to make it into 0.18
< wumpus>
I noticed it again because it has a 0.18.0 milestone
< gmaxwell>
:(
< wumpus>
same for #13103 unless the windows encoding changes already merged solve it
< wumpus>
yes that's worthwhile to test too, at least here it seems to detect the fact that the instructions aren't available correctly
< sipa>
wumpus: yes, it works for me :p
< wumpus>
I've also checked that gcc does in fact generate them
< sipa>
(at least, it reports "Using RdSeed as an additional randomness source")
< wumpus>
good!
< sipa>
i also have a system which supports rdrand but not rdseed
< sipa>
i'll test on that
< wumpus>
TIL "This instruction was introduced in the Pentium 4 processors, but is backward compatible with all IA-32 processors. In earlier IA-32 processors, the PAUSE instruction operates like a NOP instruction."
< sipa>
ha
< sipa>
i hadn't even considered that the pause instruction might have been a compatibility issue
< wumpus>
:D
< gmaxwell>
I had.
< gmaxwell>
:P
< wumpus>
I guess it's also worth testing this on x86_32, I mean, it's quite an unlikely scenario for someone with a processor that supports these instructions to run a 32-bit OS
< wumpus>
yeh Appveyor logs are quite useless usually
< jarthur>
I'm on an x86_64 macOS 10.13.6 box w/ RDRAND and RDSEED. How to test this, wumpus?
< jarthur>
Build sipa's branch and check the debug.log?
< wumpus>
jarthur: the basic thing to test would be that if you run bitcoind that you see that it's being used in the debug log
< jarthur>
thanks
< wumpus>
and ofc check if the unit tests and functional test suite pass
< wumpus>
I'm honestly not sure how to test reliability of random number generation otherwise
< jarthur>
We may be able to add some entropy scoring in the functional tests if there isn't already something like that.
< wumpus>
there isn't, at the moment
< provoostenator>
Looks like in Windows land you need individual vcpkg packages for each(?) boost component. We'll find out when it next runs... see e.g. how #14241 removed boost-interprocess
< sipa>
wumpus: seems to work fine on rdrand-not-rdseed
< wumpus>
sipa: nice!
< jarthur>
Is p2p_invalid_messages.py test known to be broken on macOS? Got a Protocol wrong type for socket error.
< jarthur>
Tested fine on rdrand-and-rdseed, either way
< jarthur>
looks like p2p_invalid_messages failure was from node closing sooner than expected and a subsequent write realizing the RST error.
< wumpus>
jarthur: on P2P? I don't think that's a known issue
< wumpus>
if you can reproduce it, please open an issue
< instagibbs>
provoostenator, achow101 re: #14021 I suppose my performance point is moot, considering most wallet operations will take a long time if hte wallet is gigantic
< instagibbs>
less stressful than running a single "listunspent"
< jarthur>
wumpus: will do, probably tonight or tomorrow
< achow101>
instagibbs: while thinking about that problem. i realized that current master is also broken in the same way
< instagibbs>
what specifically?
< achow101>
CWallet::GetKeyOrigin also does the GetKey thing and if the wallet is not unlocked, it will write a bogus master key fingerprint to the psbt
< instagibbs>
ah
< instagibbs>
welp
< achow101>
if we do it on first unlock, a user could make a psbt that has a bogus fingerprint
< achow101>
i guess we could make walletprocesspsbt throw an error if the key metadata isn't updated?
< wumpus>
jarthur: thanks
< x4b1d>
Anyone seen good code examples of offline signing outside of using bitcoind?
< wumpus>
maybe some FOSS hw wallet firmware like the trezor one?
< achow101>
wumpus: funny thing about the trezor is that the python lib's default behavior was/is(?) to call back to trezor servers to fetch prevtxs. so not offline at all!
< x4b1d>
wumpus: achow101 yeah I haven't actually seen a lot of offline signing functionality anywhee that seems to conform since 2014
< wumpus>
achow101: fair enough, but that's also true when you use bitcoind for offline signing, you have to provide the previous outputs yourself
< achow101>
x4b1d: maybe have a look at armory? it's entire goal is to do offline signing
< echeveria>
achow101: armory is highly is advisable.
< echeveria>
inadvisable. it has long standing implementation issues and a history of losing money. it’s not strongly maintained.
< wumpus>
achow101: you can't assume anything that is offline keeps up with the chain, after all, so you basically need a watch-only wallet to keep track of the utxos
< x4b1d>
doesn't armory just rpc into bitcoind ?
< wumpus>
achow101: calling into a centralized server is kinda meh though I agree
< sipa>
#bitcoin
< achow101>
x4b1d: no. it only uses bitcoind for connection to the network
< wumpus>
yea #bitcoin is better for questions like this as they don't involve bitcoin core development
< x4b1d>
Fair, I wish that I could break the offline functions into a seperate set of libs provided by bitcoin core but yeah more ot
< wumpus>
well *that* would be on topic
< wumpus>
FWIW I think that would be a good idea
< wumpus>
the RPC pure utility functions (that don't need a node, wallet) could be a library
< x4b1d>
god I wish I had that
< sipa>
libwally has a bunch of functions
< wumpus>
yes, that one is used by clightning for example
< sipa>
but a standalone library/tool that can do psbt signing would be pretty nice
< x4b1d>
sipa: yeah I've been working with libwally. But it's opinionated to support elements and I'm at the mercy of libwally to keep the standard
< x4b1d>
would be so happy to see that as a development
< achow101>
what's up with travis? the linter target is failing since python 3.4 couldn't be downloaded
< wumpus>
achow101: haven't noticed yet, where is that?
< wumpus>
curl: (22) The requested URL returned error: 403 Forbidden
< wumpus>
'forbidden' is a kind of strange error to return here, given hat it's a request from travis to travis' own archive, might be simply a access list configuration error on their end
< wumpus>
it's *before* our script takes action so nothing we can do I think
< achow101>
yeah... i'll contact their support
< wumpus>
thanks !
< jarthur>
AWS recently made it harder to grant public access to s3 resources. Have seen a few projects face perms issues on account of it.
< jarthur>
wumpus Did a little bit of due-dilligence and filed the p2p_invalid_messages issue as #15400
< gribble>
https://github.com/bitcoin/bitcoin/issues/15400 | p2p_invalid_messages test can fail in runner due to stderr text when socket write buffer flushed · Issue #15400 · bitcoin/bitcoin · GitHub
< wumpus>
jarthur: interesting
< wumpus>
we've certainly run into problems with this before, with tests failing simply due to unexpected stderr output
< wumpus>
okay this is definitely something else it happens in python itself
< jarthur>
Yea. Always hard to catch socket conditions that happen on writes, given Python's asyncio buffers the writes, then kernel buffers the transmission. We can tell asyncio to not buffer if kernel is letting us buffer everything, but kernels don't really give us any kind of guarantee.
< wumpus>
I'm still confused about #15140, I don't think we should be telling people to run clang-format if we're not even sure about that change
< wumpus>
like sometimes people are told to go through iteration after iteration of small nits while it's deeply unclear if it should be merged in the first place
< sipa>
agree
< sipa>
(in general, i haven't looked at the pr)
< wumpus>
it's been open for a while but I think it just WTFs everyone
< wumpus>
(and I don't think the issue with it was indentation)
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #15401: rpc: Actually throw help when passed invalid number of params (master...Mf1902-rpcNumArgs) https://github.com/bitcoin/bitcoin/pull/15401
< MarcoFalke>
A comment with "//---------><----- cut here"
< * MarcoFalke>
takes scissors and cuts the monitor
< wumpus>
I mean I'm slightly worried that there's an actual problem (whether they address it in the fix or not, I don't know) otherwise I'd have closed it already
< sipa>
oh, i get it
< sipa>
the test is broken
< wumpus>
oh!
< sipa>
it's testing that pushing a script hash using the OP_PUSHDATA opcodes doesn't cause it to be detected as P2SH (because BIP16 gives the exact encoding)
< sipa>
but the OP_PUSHDATA2 and OP_PUSHDATA4 opcodes are missing from the test; so they correctly fail, but for the wrong reason
< sipa>
and the cut thing is because he gives 2 versions
< wumpus>
that's where it gets confusing
< wumpus>
if the opcodes are missing, the fix would seem to be to add the opcodes, not duplicate the entire test
< sipa>
that's what his first version does
< wumpus>
right
< wumpus>
so that would seem to be enough
< wumpus>
#15257 really has a crapload of changes just to make the new version of the python linter happy