< phantomcircuit>
sdaftuar, sort of, if you have inbound connections from a peer that is itself in sync and announcing blocks then you will get headers, but won't call FindNextBlocksToDownload and thus won't get blocks
< phantomcircuit>
but if you're a listening node in ibd then you don't announce your address so you won't get inbound connections at all
< phantomcircuit>
seems like there's a number of things that should be changed but im unsure of why they're all restricted in the first place
< phantomcircuit>
well the bad headers one i am, but the no address relay im not
< sdaftuar>
if you're a listening node in ibd, getting incoming connections doesn't really help you much? really what you want is to find a node that has the honest chain. since the way you bootstrap yourself is by relaying your address to outbound peers and hoping that other people get it, you're implicitly assuming the network graph is already connected
< sdaftuar>
so i think it's sufficient for our purposes to just fix relay at the local level, and we can extrapolate out that the network will avoid getting stuck as a result
< phantomcircuit>
sdaftuar, if you're a listening node in ibd and you get an inbound connection and that inbound connection broadcasts a block then you will send the getheaders to that peer which will trigger the full headers sync logic from that peer
< sdaftuar>
right, agreed
< phantomcircuit>
but then you won't request blocks from them and you'll still be screwed, but marginally less screwed
< sdaftuar>
also agreed
< sdaftuar>
so we should fix that
< phantomcircuit>
yes, we should *also* fix that
< phantomcircuit>
but we need to broadcast our address to outbound peers in the first place to get any inbound
< sdaftuar>
while i don't think that harms anything, for the reasons i gave above, i think that is a minor consideration
< sdaftuar>
look at it this way - the miner who produced that block presumably made outbound connections to the network
< phantomcircuit>
thinking about it that might have been why i was able to just rig IBD=false and have my node sync, i instantly got hundreds of inbound connections
< aj>
can you make it so that you start broadcasting your address once you're tip is >= all your outbound peers' tips?
< sdaftuar>
as long as that miner's blocks propagate one hop, we should be able to reason that the blocks will propagate to the rest of the network
< sdaftuar>
or else nothing we do will work, if the network is partitioned
< sdaftuar>
aj: using the block count they give us in their version message?
< sdaftuar>
aj: i think that's more complicated than it needs to be. it should be okay by now to just always advertise ourselves, i think
< sdaftuar>
because peers have logic to disconnect nodes that aren't keeping up
< sdaftuar>
phantomcircuit: i would hesitate to try to learn too many lessons about addr relay in Bitcoin based on observations on altcoins
< sdaftuar>
my perspective for now at least is that the heuristics we use mostly seem to work, but whether they work on vastly different looking network graphs is totally unclear
< sdaftuar>
(mostly an aside, i think the gating of addr relay on IBD is not too significant either way)
< aj>
sdaftuar: yeah, i guess block count would be fine as a heuristic. probably right that advertising all the time is fine though
< phantomcircuit>
sdaftuar, well for sure if we don't advertise our address we're unlikely to get inbound connections
< sdaftuar>
phantomcircuit: of course. i think that is solving a distinctly different problem than block sync not working, and its worth noting that fixing block sync causes nodes to leave IBD, which mitigates these other IBD-related issues
< sdaftuar>
that said i'm all in favor of pulling out the IBD gating wherever we don't need it
< phantomcircuit>
sdaftuar, actually it looks like we might? call FindNextBlocksToDownload
< shesek>
do you think that regularly polling `getblockchaininfo` (say, every 5 seconds) to track syncing progress could have a noticeable effect on syncing times? it appears that it needs to lock cs_main
< sipa>
it does, but i don't think it will affect sync progress
< sipa>
it'll just block while it can't grab the lock, but that won't interfere with actual syncing
< shesek>
sipa, thanks for clarifying
< phantomcircuit>
shesek, getbestblockhash is a better target to see if you need to call getblockchaininfo
< shesek>
phantomcircuit, this is to display progress to the user, I'm polling getblockchaininfo until headers==blocks && !ibd
< phantomcircuit>
shesek, ah ok
< shesek>
has a pruneuntil=<height/time> config option been discussed before?
< shesek>
one currently has to start syncing, wait until the chain is indexed sufficiently, then issue `pruneblockchain`. it would be nice if this could be configured from the get-go without ongoing interaction, and also if it could prune before the height is reached to keep the peak disk usage down
< luke-jr>
shesek: you could accomplish that with prune locks
< gribble>
https://github.com/bitcoin/bitcoin/issues/20827 | During IBD, prune as much as possible until we get close to where we will eventually keep blocks by luke-jr · Pull Request #20827 · bitcoin/bitcoin · GitHub
< shesek>
nice, they both seem pretty useful
< shesek>
I think that users would typically want to specify a timestamp though, while prune locks works in terms of block heights
< sipa>
i don't understand why you'd want a timestamp?
< shesek>
it is possible to somehow ask bitcoind for the height around some timestamp?
< shesek>
sipa, I want to let users choose how much unpruned data they want to save to enable wallet rescans, they'll typically be thinking about this in terms of a date rather than height
< sipa>
ah, ok
< shesek>
I made bitcoin wallet tracker support both, users can either `--prune-until 2021-03-10` or `--prune-until 673960` (this is with trying `pruneblockchain` repeatedly until it doesn't fail)
< shesek>
could this have a negative effect on syncing times, btw? `pruneblockchain` also has to scan headers to translate the timestamp into an height
< luke-jr>
shesek: well, the idea is that when you make a new backup, you'd update the prune lock for that wallet's backup
< luke-jr>
shesek: so it won't prune until all your wallets have a backup recent enough
< luke-jr>
and if a reorg occurs, it will shift backward prune lock start heights
< luke-jr>
so in theory, this should avoid pruning ever pruning too much
< shesek>
this is also about enabling the future possibility of doing wallet rescans, say a user could choose to keep around everything since '18, when they first joined bitcoin, so it'll cover all their wallets for sure
< shesek>
basically just trying to offer some middleground between not being able to rescan at all and keeping everything
< bitcoin-git>
[bitcoin] fanquake opened pull request #21403: build: set --build when configuring packages in depends (master...explicitly_set_depends_build) https://github.com/bitcoin/bitcoin/pull/21403
< bitcoin-git>
[bitcoin] fanquake opened pull request #21405: compat: remove memcpy -> memmove backwards compatibility alias (master...remove_memcpy_back_compat) https://github.com/bitcoin/bitcoin/pull/21405
< bitcoin-git>
[bitcoin] vasild opened pull request #21407: i2p: limit the size of incoming messages (master...limit_RecvUntilTerminator) https://github.com/bitcoin/bitcoin/pull/21407
< vasild>
jonatack: how big is ./oom-da39a3ee5e6b4b0d3255bfef95601890afd80709? Can I get it in order to reproduce?
< vasild>
"base unit: 0000000000000000000000000000000000000000", does it contain just a bunch of zeros? I tried with a file that contains 20 zero bytes, but it executed without errors
< jonatack>
vasild: it is an empty file.... -rw-r--r-- 1 0 Mar 10 13:20 oom-da39a3ee5e6b4b0d3255bfef95601890afd80709
< vasild>
executes without issues here: Executed /tmp/oom in 1 ms
< vasild>
can you reproduce?
< jonatack>
do you also get OOM with? src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/
< jonatack>
(i don't have the fuzz build anymore, testing another PR, will re-check a bit later)
< vasild>
I did not try but this usage looks strange: ../qa-assets/fuzz_seed_corpus/ does not contain any seeds, just subdirectories, shouldn't that be ../qa-assets/fuzz_seed_corpus/i2p/ ?
< vasild>
I run it like "src/test/fuzz/fuzz /some/new/empty/directory"
< vasild>
anyway, I ran it over the weekend (~2 days) without seeing any issues
< luke-jr>
jnewbery: CI is supposed to merge PRs to master before running tests.. it should never require a rebase O.o
< jnewbery>
luke-jr: hmmm, i'm not sure that's working on #19391. It doesn't look like _any_ Cirrus CI jobs have been run on it
< jnewbery>
only travis and appveyor. I guess that's because the branch is from before cirrus was a thing
< aj>
i think it's just that the last update to the PR was before cirrus was a thing?
< jnewbery>
I think it's generally useful for reviewers to rebase on master fairly regularly. If you don't then they either have to rebase it themselves, or building takes forever since everything needs to be recompiled
< aj>
you don't have to rebase, just "git checkout origin/master; git merge pull/origin/12345"
< jnewbery>
aj: good tip with the git merge. As a reviewer though, I want the thing I review to be as close as possible to the thing that master would be if the branch is merged, which for this project is the branch rebased on master. Probably doesn't make that much difference though
< hebasto>
jnewbery: also `git fetch origin pull/12345/merge`
< luke-jr>
jnewbery: actually, what aj suggested is the closest thing (aside from the commit message and PGP sig)
< queip>
anyone knowing wallet code around? if you don't mind I wanted to confirm, is Core quaranteed to correctly load and show history/ballance if you load many wallet files, of which some are the same wallet but older backup of it. does the fact same key exists in many wallets is not bugging out some assumptions in code
< queip>
(if you load it all at once)
< luke-jr>
queip: you can't load more than one at a time
< queip>
luke-jr: you mean, loading older backup -wallet=wallet1.dat while also loading new version of it -wallet=wallet2.dat is not allowed?
< luke-jr>
right
< queip>
is it safe (detected and refused)?
< jnewbery>
luke-jr: oh yes, you're right
< luke-jr>
queip: yes, for bdb wallets
< queip>
luke-jr: do the wallets contain some internal UUID set on creation?
< luke-jr>
queip: yes
< queip>
oh great
< luke-jr>
the experimetnal descriptor wallets / sqlite are missing this at the moment
< luke-jr>
queip: I'm not sure when the safe detection was added
< MarcoFalke>
I generally merge/rebase a pull before review if I want to test it. My ccache is populated with latest master, so applying changes on top of master will make it compile faster
< jeremyrubin>
+ jnewbery's comment: Concept ACK, although I agree with Marco that we should wait for drahtbot to tell us how many PRs this conflicts with, and hold back if any of those are important and close to being merged.
< jeremyrubin>
recency of PR update as a proxy for importance
< MarcoFalke>
Oh you mean each conflict pr in the list would have a (last updated: date). That is certainly possible, but would need to be updated on probably every run
< MarcoFalke>
you mean inverse proxy? ;)
< jeremyrubin>
'heuristic'
< MarcoFalke>
For example #20966 is important, but it doesn't have any comments (apart from "Needs rebase" and "Rebased")
< jeremyrubin>
it's not super important, I've found myself wanting this before though when I get random conflicts. I could see how it would be annoying to have to go back through and filter updates by drahbot/non drahbot etc
< jeremyrubin>
MarcoFalke: is drahbot open source somewhere?
< MarcoFalke>
The code is too ugly to make open source
< MarcoFalke>
You'd be able to write nicer code in less than 5 minutes
< jeremyrubin>
invite me to the repo I could take a look...
< jeremyrubin>
Maybe counting commit tagged ACKs for the current git tip could be a good heuristic too?
< jeremyrubin>
MarcoFalke: not the hugest fan of devops-y stuff so it would probably take me > 5 mins :)
< MarcoFalke>
Yeah, the ACK would be a good idea
< MarcoFalke>
*ACKs
< jeremyrubin>
Maybe also adding the ability for maintainers / PR author to comment clearack if they think the acks need to be invalidated (e.g., someone finds a bug)
< jeremyrubin>
don't need to get too fancy
< jeremyrubin>
but I think improving the tooling a bit could help deduplicate a lot of replicated manual work
< MarcoFalke>
heh, all I know about devops is how to write a loop in bash. DrahtBot is basically a `while true; ./label_rebase.py ; done`, where the python script just imports a github api library and then tags all pulls needing rebase...
< MarcoFalke>
That's what NACK is for
< jeremyrubin>
I think NACK is ambiguous between NACK this and all bugfixes of this v.s. "the current ACKs should be invalidated"
< jeremyrubin>
But I guess it's not too important
< MarcoFalke>
Concept NACK / Approach NACK are about the concept/Approach
< MarcoFalke>
NACK commit_id is about a specific id
< MarcoFalke>
(at least that's how I use it)
< MarcoFalke>
Probably should be documented as such
< sipa>
yes
< jeremyrubin>
yeah
< jeremyrubin>
Could be nice to have an @admiralackbot that has a documented ACKs API to better collate and style conform what ACKs have been used... GH also has builtin "approved changes" but I don't find it explicit enough for my tastes
< jeremyrubin>
Maybe I'll hack one together if people like the idea of having something like that
< jeremyrubin>
MarcoFalke: there's some annoying things with how git works that it probably makes sense to clarify around e.g., rebase + push -f that would make NACK id stale that we should clarify NACK <id> becomes NACK untill explicitly lifted
< jeremyrubin>
I made a comment as such but it feels... overkill
< MarcoFalke>
jeremyrubin: You can re-NACK (just like a re-ACK)
< jeremyrubin>
I think on the balance it's safer to require re-ack, but not to require re-nack
< MarcoFalke>
Also, if you use the GitHub NACK feature, the merge button will turn red (instead of green)
< MarcoFalke>
Even after rebase
< MarcoFalke>
I think it is safe to assume a NACK is "sticky" until unsticked (no need to document that)
< achow101>
queip: if you load multiple wallets with the same keys, it will handle them independently. It will show all of the same transactions, although the timestamps of those transactions may differ. If you copy the wallet file, for bdb wallets, we don't allow you to load the copies due to caching (and other) issues with bdb itself. There is no other reason that loading duplicate wallets would be disallowed.
< MarcoFalke>
Obviously to be on the safe side, I'd still encourage to re-NACK if needed.
< MarcoFalke>
If the documentation is too verbose, chances are less people will read it
< MarcoFalke>
what about adding labels to denote the number of ACKs?
< jeremyrubin>
if people abuse it we can make it only count ACKs on a list (just for the tooling, people could add themselves) but I think ACK labels are great
< luke-jr>
MarcoFalke: the label should be coloured lime green /s
< MarcoFalke>
luke-jr: Did that
< luke-jr>
lol
< jeremyrubin>
a different shade though
< jeremyrubin>
(jk)
< luke-jr>
jeremyrubin: wait, you mean it should be key-lime green?
< jeremyrubin>
Mmm was thinking chartreuse actually
< luke-jr>
wut
< jeremyrubin>
it comes from the french liquor chartreuse, which comes from "charter house", where a charter can be a conferrence of privilege.
< jonatack>
nah, these liquors usually were made by monks or nuns
< jonatack>
this one by carthusian monks located in the chartreuse mountains near the french alps
< wumpus>
it's an interesting idea but i'm not really sure using labels for something like counting acks is a good idea, it really goes aside of the idea of labeling
< wumpus>
there are already so many labels
< wumpus>
ui-wise it's basically a more limited version of what bitcoinacks is doing but without the names
< wumpus>
just 'number of acks' is also such an easy metric to game, if you're going to merge it's probably more important to pay attention to the rest
< bitcoin-git>
[bitcoin] jonatack opened pull request #21410: test: increase rpc_timeout for fundrawtx test_transaction_too_large (master...give-test_transaction_too_large-more-time) https://github.com/bitcoin/bitcoin/pull/21410
< bitcoin-git>
[bitcoin] jonatack opened pull request #21411: test: add logging, reduce blocks generated in wallet groups test (master...wallet_groups-test) https://github.com/bitcoin/bitcoin/pull/21411
< fanquake>
I agree with wumpus, not sure about the value of “n ACKs” labels.
< fanquake>
Also, please don’t have drahtbot start adding and removing ACK labels because some spam/unknown accounts drive by and drop ACKs on things. That’s just more noise.
< fanquake>
I think jeremyrubin idea of having some sort of last updated time in the conflict comment is a good one.
< phantomcircuit>
wumpus, thoughts on removing zmq? afaict 100% of the users of it are using it in a way that is either dangerous or requires low interval polling anyways
< bitcoin-git>
[bitcoin] glozow opened pull request #21413: [rfc] add option to bypass contextual timelocks in testmempoolaccept? (master...2021-03-bypass-timelocks) https://github.com/bitcoin/bitcoin/pull/21413