< sdaftuar_>
jonasschnelli: that is a very strange failure. it looks like the test is trying to construct a 1000000byte block to test p2sh sigop counting, but somehow in your test failure the block was 1000001 bytes, which is why it broke
< sdaftuar_>
jonasschnelli: maybe there is some non-determinism in the test that we need to track down
< bitcoin-git>
[bitcoin] achow101 opened pull request #16463: [BIP 174] Implement serialization support for GLOBAL_XPUB field. (master...bip174-xpub) https://github.com/bitcoin/bitcoin/pull/16463
< jnewbery_>
Does anyone know the probabilities of 70 or 69 byte signatures? Is it always 1/2n of the number of bytes under 73?
< harding>
jnewbery_: I think 73 -> 72 is a special case (because an extra byte is added if the value is over 0x80). For anything below 72, there would have to be a 0x00 byte in order for it to be implicit, so the odds are 1/256 per each byte fewer. So 70 bytes or fewer would be (1/2 * 1/256 * 1/256). I'm not confident about this, though.
< sdaftuar_>
jonasschnelli: i think i found the problem
< jonasschnelli>
sdaftuar_: nice! What is it?
< sdaftuar_>
there's non-determinism in the length of the signature in the first transaction of the block (b39) used in the first p2sh sigops test
< sdaftuar_>
combined with a bug in the way the block size is measured in the loop, we can accidentally make too big a block if the first transaction has a too-small signature
< sdaftuar_>
i've got a simple fix, i think, which i'm verifying
< jonasschnelli>
Cool! Thanks for fixing it sdaftuar_.
< jonasschnelli>
So it should have also happend in travis
< jonasschnelli>
(it probably did)
< sdaftuar_>
yeah that's what motivated john's question, we were trying to figure out what the likelihood of this happening is
< jonasschnelli>
i see
< sdaftuar_>
i suspect the likelihood went up after we switched to using a new secp256k1 implementation for our python test suite?
< sdaftuar_>
sipa: ^^ does that make sense to you?
< sipa>
sdaftuar_: the python EC code doesn't do low-r grinding, so it'd be around a 50% chance for 71 byte sigs and 50% for 72 byte sigs (including the sighash byte)
< sipa>
and a tiny probability for 70 bytes and below
< emilengler>
In which file/function the inital blockchain download is being started?
< sipa>
emilengler: PeerValidationLogic::SendMessages(CNode* pto) in src/net_processing.cpp
< sipa>
specifically under the comment "// Start block sync"
< sipa>
it's a function that gets called periodically for each peer
< sipa>
that piece of code specifically starts the fetching of headers
< sipa>
once we have enough headers, and know of peers who have blocks corresponding to those headers, we'll automatically start requesting blocks from them
< emilengler>
sipa, Thank you
< bitcoin-git>
[bitcoin] sdaftuar opened pull request #16464: [qa] Ensure we don't generate a too-big block in p2sh sigops test (master...2019-07-fix-feature-block) https://github.com/bitcoin/bitcoin/pull/16464
< jnewbery_>
meeting time?
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jul 25 19:00:20 2019 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< ariard>
(current tip ACKed by jnewbery and jonatack)
< MarcoFalke>
ariard: Looks like you pushed a new commit today
< achow101>
#16341 for Chasing Concept ACK?
< gribble>
https://github.com/bitcoin/bitcoin/issues/16341 | WIP: Introduce ScriptPubKeyMan interface and use it for key and script management (aka wallet boxes) by achow101 · Pull Request #16341 · bitcoin/bitcoin · GitHub
< wumpus>
good to know, I see it's already on there
< MarcoFalke>
Oh, I see they re-ACKed
< ariard>
I took jnewbery cleanup commit for BroadcastTransaction
< MarcoFalke>
ariard: It conflicts with #16452. So which one should go in first?
< amiti>
tldr; want to improve privacy with updates to rebroadcast logic
< amiti>
instead of nodes rebroadcasting only wallet txns, rebroadcast all txns it believes should have been in the last block
< amiti>
looking for critical feedback, concept acks, any high level implementation thoughts
< wumpus>
how much is this expected to increase bandwidth usage?
< amiti>
choosing the parameters carefully will be important - dramatically impacts the worst case bandwidth usage
< wumpus>
and does this help with privacy? the first node broadcasting something will still be the same one
< wumpus>
as I see it, at least
< amiti>
not necessarily. if a node has a txn in its mempool that should have been picked up in a block already (high fee rate and is old), it will rebroadcast. independent of originating wallet.
< MarcoFalke>
wumpus: Yes, the first node (i.e. the wallet) will be the same one
< MarcoFalke>
But hopefully not for rebroadcasts
< jnewbery_>
wumpus: I think it does. Currently, if you see a peer brodcast a tx more than once, you can be almost certain that it originated the tx, and we rebroadcast our txs that are unconfirmed for more than half an hour
< sipa>
i think it primarily addresses how currently we gratuitously rebroadcast, making it obvious continuously to every peer what our own transactions are
< sipa>
it certainly doesn't address all forms of leaking that information
< MarcoFalke>
small steps :)
< wumpus>
so either every node does this, or it reveals which nodes have a hot wallet
< sipa>
wumpus: the rebroadcast logic will be in the mempool, so even without wallet enabled, it will work
< wumpus>
and if all nodes do it, that sounds very bad for bandwidth usage
< achow101>
at first glance, this sounds like it will cause bandwidth usage greatly increase
< sdaftuar>
wumpus: i think if we constrain it to old transactions at the top of themempool, it should be a small bandwidth effect?
< sipa>
i suspect it's also filted by the knowninvs logic, so we wouldn't rebroadcast to the same peer twice
< wumpus>
so basically all nodes would create noise for the nodes with a wallet to hide in, hmm
< MarcoFalke>
sipa: The knowninvs will reset every couple of blocks, no?
< sdaftuar>
wumpus: i think it's more than that, it's way to ensure that things that should be mined get relayed, eg to nodes with small mempools or recently started up
< sdaftuar>
sort of like a mempool-sync might do
< achow101>
would it be picking the oldest high fee rate transactions to rebroadcast, up to some n? Or would it pick some n transactions and choose the oldest and highest fee rate of them?
< wumpus>
sdaftuar: oh good point
< sipa>
right, full nodes have an incentives themselves to see their mempools match what it actually mined
< sipa>
even without wallets
< amiti>
achow: traverse top of mempool by ancestor fee rate, filter out recent transactions, rebroadcast remaining
< jnewbery_>
if we consider the top of the mempool to be "transactions we expect to get mined soon" and they're not getting mined, rebroadcasting them to make sure miners are aware seems like sensible mempool logic
< jnewbery_>
if not, then the mempool might as well expire them - they're doing our node no good
< sipa>
i guess there could be some sort of exponential backoff, that after a tx has been rebroadcast multiple times, it becomes rarer (this may be especially relevant when peers may have other consensus/policy rules)
< achow101>
amiti: so every rebroadcast would have the same number of transactions?
< sipa>
though i guess that's done automatically by our own expiration logic
< achow101>
or rather same amount of data being broadcast
< wumpus>
so if every node broadcasts the transactions at the top of the mempool, this will be more or less the same transactions for every node
< MarcoFalke>
achow101: No. Txs that were added to the mempool or to blocks are excluded
< sipa>
achow101: i guess that depends on how well the mempool matches what is being mined
< MarcoFalke>
wumpus: Yes, mostly
< wumpus>
if someone broadcasts a *different* transaction, or one that would rank lower according to policy, this is suspect
< sdaftuar>
sipa: i think a rebroadcast cap makes sense, also a cap on the number of things rebroadcast in one go (to prevent any kind of edge-case behavior)
< sipa>
gleb: here?
< sdaftuar>
he's away
< sipa>
i'm wondering how to integrate this with erlay
< sipa>
(which shouldn't be a blocker for this discussion of course, but it's nice to have things thought out)
< wumpus>
I'm not entirely sure that this generates noise with enough variance to contribute to privacy
< sipa>
wumpus: hmm?
< wumpus>
it'd just change the logic to 'anyone that broadcasts a transaction that is not on the top of the mempool is broadcasting their own transaction'
< sipa>
is your concern "this isn't enough" or "this doesn't contribute anything" (possible)
< provoostenator>
In particular, this wouldn't benefit lowball fee transactions I assume?
< MarcoFalke>
wumpus: The wallet rebroacast would be removed of course
< jnewbery_>
wumpus: nodes would still relay new transactions they saw
< wumpus>
MarcoFalke: ohhh!
< MarcoFalke>
heh
< wumpus>
MarcoFalke: okay then it makes a lot more sense
< MarcoFalke>
Maybe we should introduce each topic with a three line summary
< provoostenator>
(I mean: I would not add more privacy to wallet transactions with a low fee, but also doesn't make it worse)
< * MarcoFalke>
meta
< provoostenator>
*it
< achow101>
how would this interact with prioritizetransaction? I assume it would be a privacy leak that you prioritize some particular transaction if that transaction was part of the rebroadcast but not the top for other nodes
< MarcoFalke>
provoostenator: The fee rate should have no effect on privacy after the proposal is merged, no?
< amiti>
provoostenator: my proposed changes would not rebroadcast wallet txns with a low fee until that txn makes it to the top of the mempool
< sipa>
hmm, so how will low-feerate transactions get broadcast at all?
< sdaftuar>
achow101: i think we'd have to use a sort on actual feerate, and not modified feerate
< MarcoFalke>
sipa: When they are created
< sdaftuar>
sipa: pacakge relay, or wait til they get to the top of the mempool, i think?
< MarcoFalke>
(once)
< sipa>
oh, nvm; the normal "just entered the mempool" relay will do that, not rebroadcast logic
< amiti>
sdaftuar: correct
< provoostenator>
So IIUC (I should read the whole thing): we still broadcast the first time as per usual, but we only *rebroadcast* top of mempool?
< harding>
So if I create a low fee transaction that doesn't get relayed initially for some reason, my wallet would never broadcast it unless I just happened to have my node open at the point where fees have dropped?
< amiti>
provoostenator: correct
< wumpus>
harding: it would broadcast it the first time
< harding>
wumpus: right, but what if it wasn't relayed the first time?
< achow101>
harding: I think it's the same situation now. you'd still have to wait for the fees to drop and have your wallet be open so that other nodes will accept it
< MarcoFalke>
harding: Right, but we can improve inital relay as well
< MarcoFalke>
(workin on it)TM
< wumpus>
concept ACK on the idea from me
< harding>
achow101: to overcome the dynamic minimum relay fee, that's true. I'm thinking of the case where I generate a transaction with a (say) 288-block estimatesmartfee target. That's only rarely going to be near the top of the mempool. Right now, my wallet will rebroadcast that every hour or so (random delay); with this change, it'd only rebroadcast it if my wallet was open at the right time.
< sipa>
harding: but on the other hand, your peers will do the rebroadcast for you
< sipa>
those who heard the initial broadcast at least
< sipa>
you can argue that that's relying on their altruistic behaviour, but right now we're already kinda doing that hoping for them to relay it at all
< sdaftuar>
it seems reasonable for the wallet to try to ensure that the transaction was relayed to at least one peer, perhaps
< harding>
An actual usecase of my is that I sendrawtransaction spends from my cold wallet with my network disabled so that I can do a final inspection of the transaction in my local mempool (mainly to check that I didn't forget an output and spend all to fees). That means I always use wallet rebroadcasting in those cases.
< jnewbery_>
harding: s/it'd only rebroadcast it if my wallet was open at the right time/it'd only rebroadcast if your node was online at the right time
< jnewbery_>
but I think your point stil lstands
< achow101>
harding: use testmempoolaccept instead?
< sipa>
harding: that seems like a reasonable use case (though personally i'm using analyzepsbt for that now, before even broadcasting it)
< amiti>
harding: you are right. in the circumstance where a low fee rate txn was not relayed the first time and none of your peers have it, these changes would make it take longer until your txn got rebroadcast, and potentially your node has to be online the right time.
< sipa>
i do think we need a way for this to work when a tx is submitted to your mempool while you're offline
< provoostenator>
One consideration I've seen discussed in a PR recently is that the node doesn't tell the wallet what happened.
< harding>
sipa: I also do that now too. When dealing with thousands of my money, it's belts, suspenders, extra belts, and extra suspenders. :-)
< provoostenator>
And one thing to also consider is that with dynamic loading, a user might unload their wallet after initial broadcast.
< sipa>
wow thousands of moneys
< sdaftuar>
a lot of ico's these days
< sipa>
the mempool could have a per-tx flag "ever broadcast"
< MarcoFalke>
Could the mempool keep track if the txs was pushed to at least one peer?
< sipa>
jinx
< wumpus>
IIRC the wallet used to keep track of number of broadcasts of a transaction, this was removed at some point
< sipa>
in that case, even the initial broadcast of a tx could become pure mempool responsibility
< sipa>
wumpus: correct
< achow101>
sipa: isn't it already?
< sipa>
though it was just used for showing in the UI whether a tx had propagated
< MarcoFalke>
wumpus: That wouldn't work if the wallet is on a different node than the mempool
< sdaftuar>
sipa: if you're the last of your peers to learn of a transaction, was it "ever broadcast"?
< wumpus>
which wasa arguably somewhat useful :) but yes
< sipa>
achow101: well i mean *the mempool* rather than ATMP
< sipa>
sdaftuar: anything you've learned from the network would never get that flag, only rpc/wallet submissions
< MarcoFalke>
sdaftuar: If you get the tx from the network it was broadcast
< wumpus>
MarcoFalke: is that even possible?
< wumpus>
the wallet will always submit the transaction to the local mempool first right?
< sipa>
right
< MarcoFalke>
Sure. Use signrawtransaction on the one with the wallet and sendrawtransaction on the one with the mempool
< wumpus>
oh sure, but in that case you're handling everything manually anyway
< provoostenator>
Well, that makes the sendrawtransaction node's mempool is responsible?
< wumpus>
yes
< MarcoFalke>
You'd still want your node to broadcast at least once (even if at the time of ATMP you were offline)
< sipa>
right now if you use sendrawtransaction, it gets submitted to the mempool/network directly, from which your own wallet may learn it, who will then take over rebroadcasting
< sipa>
because sendrawtransaction is a node RPC not a wallet RPC afaik
< wumpus>
correct
< wumpus>
there's no addrawwallettransaction or something like that
< wumpus>
sendrawtransaction will unconditionally broadcast the transaction as well, so it's sometimes used for manual rebroadcast
< MarcoFalke>
sendrawtransaction should mention that this is privacy leaking, maybe?
< wumpus>
yes, the help could mention that
< sipa>
amiti: what do you think about the "ever broadcast" flag idea?
< provoostenator>
From the gist: "The wallet will attempt to resubmit unconfirmed transactions to the node on a scheduled timer" - does the node remember previously dropped txs?
< sipa>
provoostenator: the wallet always remembers all its own transactions
< amiti>
sipa: sounds great! noted.
< sipa>
the mempool is completely neutral and doesn't treat our own txn different from anyone else's
< provoostenator>
sipa: yes, but wouldn't this behavior just cause the node to do a fresh broadcast of all wallet transactions?
< amiti>
well. if we add the flag, it wont be completely neutral anymore
< MarcoFalke>
sipa: I like it. It sounds even orthogonal to amiti's proposed change
< sipa>
MarcoFalke: indeed
< sipa>
amiti: good point
< sipa>
but at some point we have to treat our own txn different our they wouldn't be broadcast at all :)
< jnewbery_>
sipa: we're wondering if the wallet rebroadcasting txs that it learns from the mempool is also true for txs received over p2p
< amiti>
but, given the circumstances harding pointed out, it seems worth it
< jnewbery_>
ie that your wallet can be dust-attacked and it'll start rebroadcasting the dust txs
< MarcoFalke>
jnewbery_: It should
< MarcoFalke>
s/should/probably does/
< sipa>
jnewbery_: i think a tx learned through the network (wallet or not) should not be treated differently
< sipa>
only things learned from the wallet or rpc, and never broadcast before
< MarcoFalke>
jnewbery_: I think this has been done in the past
< amiti>
provoostenator: if I understand your question correctly, the node will not remember the txns it previously dropped. it would rely on the wallet to resubmit if needed
< MarcoFalke>
Oh, no. It has been done to get the coinselection include the dust
< jnewbery_>
I guess we want this flag to be saved to mempool.dat so we don't rebroadcast our own txs every time we restart
< sipa>
jnewbery_: yeah
< sipa>
that's scary because mempool saving is only done periodically (or even only at shutdown)
< sipa>
so an unclean shutdown could result in unnecessary rebroadcast
< MarcoFalke>
-nopersistmempool :eyses:
< provoostenator>
Unnecessary rebroadcast as a result of a crash doesn't seem like a huge issue.
< MarcoFalke>
rebroadcast on every start because you don't persist the mempool does sound like an issue
< harding>
You could put the flag on the transaction in the wallet instead? was_relayed_to_at_least_one_peer
< sipa>
harding: a bit of a layer violation but yeah
< MarcoFalke>
harding: That wouldn't work where the wallet is separate from the mempool (two nodes)
< sipa>
it means that the "submit to mempool" function needs to return a bool broadcast or not
< sipa>
oh right, it doesn't work for RPC
< provoostenator>
Maybe the wallet could ask the node "Do you have this in your mempool?"
< provoostenator>
And maybe also "Put in your mempool, but skip initial broadcast"
< sipa>
right but it needs to work even when there is no wallet involved at all
< provoostenator>
That moves responsibility to the wallet, which maybe makes sense because it cares about its own privacy.
< provoostenator>
sipa: true, that's the downside
< MarcoFalke>
We should remove broadcast logic from the wallet and the rpc and tell users to copy-paste the tx into a block explorer over tor
< wumpus>
at least if their node is not connected to tor already
< hugohn>
question: if there are empty blocks (could be consecutive), will every node rebroadcast top of the mempool?
< jonasschnelli>
It's a custom built open source CI that is/can-be-further tailored to our needs. Super slim.
< MarcoFalke>
hugohn: I think so
< jonasschnelli>
bitcoinbuilds.org
< jonasschnelli>
Feature wise its on the same level then travis. Builds fast or even faster then travis on a 50EUR/month machine.
< jonasschnelli>
Additionally, one can download the build results and it logs times per task (install/compile-depends/configure/compile/run-tests/etc.)
< jonasschnelli>
Intention is not to replace travis. Just to have a backup option for times when we need it.
< wumpus>
so we can trash travis now? :)
< wumpus>
oh
< jonasschnelli>
Maybe at some point.. but not now
< jonasschnelli>
I added a github hook.
< wumpus>
github integration is probably the most difficult part
< jonasschnelli>
So it builds are PRs (master after merges soon)
< jonasschnelli>
Integration with github is easy
< wumpus>
(e.g. reporting the status on github)
< wumpus>
oh! it used to be pretty much impossible for custom tools
< jonasschnelli>
In general... I was surprised how easy it is to "clone travis"
< MarcoFalke>
reply from travis: "Thank you for getting in touch and for raising this issue as well. I apologise for the frustration caused over the last month regarding the problems, and I can assure you we are committed to improving your overall experience while using the platform."
< jonasschnelli>
(though tailored)
< jonasschnelli>
However,.. feel free to check your PR build on bitcoinbuilds.org and contribute to futher extend it to our needs.
< meshcollider>
Currently it just compiles I guess, are you planning on adding test execution?
< jonasschnelli>
cfields more detailed dependency cache would certainly be a build-performance booster (for some types of builds)
< wumpus>
MarcoFalke: hopefully that means anything
< jonasschnelli>
meshcollider; it runs the test on linux64/32
< MarcoFalke>
wumpus: I'd guess its a template response
< meshcollider>
Oh nice :)
< jonasschnelli>
I have also plans to allow running tests on other machines over the internet (like run the ARM tests on a odroid or so)
< MarcoFalke>
Nice
< jonasschnelli>
Contributions welcome...
< jnewbery_>
jonasschnelli: does it use the same travis.yaml format for configuration?
< jonasschnelli>
It's static for now to not pollute our git
< wumpus>
oh that's neat!
< sipa>
ideally our travis.yml doesn't really contain any logic apart from "invoke CI step 1", "invoke CI step 2.a", and caching ... which are implemented as a shell script
< jonasschnelli>
yes
< jnewbery_>
sipa: for logic yes, but there's various config in there too
< MarcoFalke>
Some of the variables in the script are travis specific, but it shouldn't be too hard to replace them
< sipa>
right
< MarcoFalke>
Happy to review a pull if someone wants to pull them out
< jonasschnelli>
We could use the .travis folder in the custom CI,.. sure.
< jnewbery_>
jonasschnelli: thanks for doing this! Definitely useful to have an alternative
< instagibbs>
oh whoops, meeting, I thought you guys were really just havin a productive normal discussion
< jonasschnelli>
heh
< amiti>
thank you everyone for all the feedback :)
< jonasschnelli>
jnewbery_: also, eventually travis builds other stuff then bitcoinbuilds.org does (if both are running fine)...
< jonasschnelli>
so the .travis file doesn't need to be directly synced
< hugohn>
amiti: should there be a check for minimum of empty blocks seen before triggering rebroadcast ? seems bad to have every node rebroadcast top of the mempool as soon as there's one empty block. (Granted, empty blocks would not be a problem post-subsidy, but might still be a problem now.)
< instagibbs>
single empty blocks are still quite common, not due to lack of mempool contents
< instagibbs>
(I haven't read the latest proposal, sorry)
< sipa>
if there is per-tx mempool tracking anyway, there could be a "point sysyem" where every time a tx was expected to have been in a block but wasn't, it gets a point; if the number of points exceeds some threshold, rebroadcast and increment a counter that next time more points are needed
< lightlike>
is it also common that non-empty blocks are mined that include many low-fee txes even if higher ones are available? or is it mostly either empty or fee-efficient?
< bitcoin-git>
bitcoin/master 248e22b fanquake: depends: disable unused Qt features
< bitcoin-git>
bitcoin/master a54a120 Wladimir J. van der Laan: Merge #16386: depends: disable unused Qt features
< amiti>
hugohn: the current thinking is not to trigger rebroadcast based on block timing, but rather an independent timer. the idea is if we choose the param of "is txn recent" to be long enough, we should be able to avoid excessive broadcast of txns even in edge cases.
< amiti>
sipa: thats an interesting idea. I'll add it to considerations. there are a lot of options for strategies to mitigate excessive rebroadcasting (and bandwidth usage) with the main tradeoffs being memory / code complexity. another way of storing per-tx mempool data would be a stamp of the `last_rebroadcast_at` and enforce a minimum there.
< hugohn>
lightlike: high-fee txns could get skipped simply due to timing/relaying issues, so I can see that happening. In fact, it sounds like amiti's proposal is geared towards solving that kind of scenario. Normally you would not expect miners to skip high-fee txns as it does not make economic sense to do so.
< wumpus>
MarcoFalke: looks like it's not picking up the "needs gitian build" on #16441
< MarcoFalke>
FileNotFoundError: [Errno 2] No such file or directory: 'make': 'make'
< MarcoFalke>
I transferred to a new vm. I guess the host needs make to make the depends
< jonasschnelli>
lol
< wumpus>
hehe
< MarcoFalke>
Fixed. Let's check back in two days. (it is half a cpu)
< hugohn>
> not to trigger rebroadcast based on block timing, but rather an independent timer
< hugohn>
amiti: I see, looks like rebroadcast is triggered once / hour based on the current proposal. Would there be an issue though if there is a sudden drop in hash rate? I suppose an hour should give plenty of room to account for hash rate drop.
< wumpus>
MarcoFalke: thanks!
< elichai2>
jonasschnelli: btw, this might interest you: https://github.com/drone/drone it's also open source and it's dockerized, I used it in the past when I wanted to host my own CI, so you don't have to pay anything except your server(then you can take a powerful one for fast compilation time)
< amiti>
hugohn: I think more relevant than the frequency of the rebroadcast job is the definition of "recent" transaction. the current proposal has that defined as 30 minutes, which may be too low.
< amiti>
right now with the params chosen (enforce max rebroadcast of 1000 txns/hour) my back-of-the-envelope math has the worst case as 36 kb max of inv message / hour.
< amiti>
I'd be curious to hear evaluations of how impactful that seems.
< amiti>
The worst case bandwidth usage would be very tune-able by choosing conservative params.
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #16465: test: Test p2sh-witness and bech32 in wallet_import_rescan (master...1907-testAllAddressTypesImport) https://github.com/bitcoin/bitcoin/pull/16465