< wumpus>
amiti: sure but i think it's important to allow for flexibility, we don't really know all uses people have for the software, this is even more the case for open source software ... as long as it doesn't result in a direct bug / crash i don't see why it should be prevented
< wumpus>
amiti: in this case there isn't any harm caused if someone does `-connect -dnsseed=1`, at most it wastes some bandwidth
< wumpus>
software like bitcoind is primarily a tool, it's in the end not really up to the maintainers how people use it, there's some actually dangerous combinations that are possible: e.g. we strongly recommend that people don't bind RPC on anything but localhost, but if you really want to they can, and i guess there might be some legit scenarios where someone wants to do it
< wumpus>
and yes, people might 'accidentally misunderstand how CLI flags work', but that's more of a risk for real footguns
< wumpus>
dhruvm: i somewhat wonder what specifically made you concerned about this combination in the first place
< wumpus>
also: maybe your node doesn't care about the addresses itself, but your node still takes part in address gossiping, so AddrMan functionality shouldn't be fully disabled, that'd be a logically separate choice
< wumpus>
in any case a default is sufficient here, doing anything else would *add* complexity
< wumpus>
the higher level question would be 'should AddrMan be entirely disabled in some cases', we had some related discussion wrt. that for BIP155 for making it possible for nodes to signal that they don't take part in address gossip
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #20065: fuzz: Configure check for main function (master...2010-fuzzMainConfig) https://github.com/bitcoin/bitcoin/pull/20065
< bitcoin-git>
[bitcoin] theStack opened pull request #20067: refactor: remove use of boost::algorithm::replace_first (master...20201003-get-rid-of-boost-replace_first) https://github.com/bitcoin/bitcoin/pull/20067
< wumpus>
hebasto: no, the script for merging doesn't check this, it's up to the maintainer merging it to do such checks or not
< wumpus>
how deep is this linting script checking anyway? I'm a bit surprised it checks previous PRs, I don't think it should
< aj>
wumpus: it's $TRAVIS_COMMIT_RANGE which is meant to be only the PR commits, but is all the commits from both the PR and that were merged since the PR's branch point, i think -- https://github.com/travis-ci/travis-ci/issues/4596 also #19654
< sipa>
fjahr: i haven't followed this; what is the problem with f471a3be ?
< fjahr>
it has "scripted diff" instead of "scripted-diff" as a prefix, which makes the linter fail. But it was merged and since the linter uses TRAVIS_COMMIT_RANGE it checks all the pushed commits of a new PR. So when a new PR is opened all commits are checked, including the offending one in master.
< sipa>
oh
< sipa>
and the appveyor build is failing on #20071, for the ubrelated msys2 problem, sigh!
< hebasto>
travis environment seems inconsistent; both #20072 and #20073 are built on the same base, but TRAVIS_COMMIT_RANGE seems have different base commit for them
< hebasto>
fjahr: that is not the case for the recent 20072 and 20073
< hebasto>
your 20072 has the same base commit as my 20073
< fjahr>
I am not sure about the initial pushes when a PR is first opened. But since I saw my new or fail I assumed it goes back far, maybe even all commits.
< fjahr>
that they are the same would make sense for new commits
< sipa>
54fc96ffa70ad3a53d32709101b7a2ce064d822e...f006f1679d0833f8d9c693553196a68a69e4f65f on #20073
< sipa>
which is just master and the head of 20073
< sipa>
try pushing a version of 20073, rebase on an older version of master?
< fjahr>
a true, so maybe initial push is from master if your pr branch is up to date on master and if it's not up to date it is all commits from the last commit in the branch
< fjahr>
*is from master HEAD
< sipa>
note the ... syntax
< sipa>
which does not mean "
< sipa>
what you may expect
< sipa>
it's all commits that are in A's history but not in B's, or the other way around (for A...B)
< sipa>
if A and B have different snapshots of master in it, the difference between the two will be included in the range
< hebasto>
sipa: experimental #20073 rebased on older base
< hebasto>
failed with value 54fc96ffa70ad3a53d32709101b7a2ce064d822e...202ea04949ea9b742fa3368bb9104bb3a2099d3d
< sipa>
which means that any commits in master that your PR has not been rebased on, are included
< hebasto>
the latest TRAVIS_COMMIT_RANGE value is wrong; the base commit is 597488d37c9c358837616516d88f861f5c25f827
< sipa>
i don't think it's wrong - it's doing what it's designed to, but that's not what we need
< hebasto>
I'm wrong; the first is master commit
< hebasto>
as it is `...` syntax
< sipa>
fjahr: concept ack 20071
< fjahr>
sipa: ty
< hebasto>
sipa: I don't see how the commit range `54fc96ffa70ad3a53d32709101b7a2ce064d822e...202ea04949ea9b742fa3368bb9104bb3a2099d3d` includes the offender f471a3be00c2b6433b8c258b716982c0539da13f
< sipa>
you're going to update it to also use that mechanism for the whitespace linter?
< hebasto>
sipa: as the common commit is 597488d37c9c358837616516d88f861f5c25f827, which is later than offender
< sipa>
hebasto: the concept of X...Y is that if you have a git history of the form r-c1-c2-c3-X and r-c1-c4-Y, then X...Y is the set of commits (c2, c3, c4, X, Y)
< sipa>
it is not a "range"
< sipa>
it's a set difference
< hebasto>
I understand this notion
< hebasto>
that why I'm asking
< sipa>
hebasto: 54fc96ffa70ad3a53d32709101b7a2ce064d822e has the offender f471a3be00c2b6433b8c258b716982c0539da13f in its history; 202ea04949ea9b742fa3368bb9104bb3a2099d3d does not
< fjahr>
sipa: I will unless 20071 already gets merged. But will finish the whitespace one soon.
< sipa>
hebasto: which also means the merge-base 597488d37c9c358837616516d88f861f5c25f827 does not have the offender in its history
< sipa>
so i think you're just overlooking something
< hebasto>
sipa: correct
< sipa>
fjahr: i believe your code in 20071 is unnecessary; "$MERGE_BASE..HEAD" should be identical to "$(git merge-base HEAD $MERGE_BASE)..HEAD", unless there is a subtle difference between master and $MERGE_BASE ?
< hebasto>
sipa: that means TRAVIS_COMMIT_RANGE for a new pr which is built on top of current HEAD (that merges the offender) should not include the offender, right?
< sipa>
hebasto: correct
< sipa>
that's my understanding, but i'm not entirely sure that re-pushes don't have extra behavior
< hebasto>
sipa: thanks
< sipa>
fjahr: the merge-base with master may result in weird effects if it's a PR for a release branch rather than master
< fjahr>
sipa: I am not sure if I understand. Did you mean it should be identical to "$(git merge-base HEAD)..HEAD"?
< fjahr>
Otherwise I am confused because you still have $MERGE_BASE in there
< sipa>
fjahr: i'm saying that you should use "$MERGE_BASE..HEAD" and not "$(git merge-base HEAD $MERGE_BASE)..HEAD" as you're doing now
< sipa>
sorry
< sipa>
fjahr: i'm saying that you should use "$MERGE_BASE..HEAD" and not "$(git merge-base HEAD master)..HEAD" as you're doing now
< sipa>
what you're doing now is equivalent (as far as I understand) to "master..HEAD", which would be wrong for non-master PRs
< fjahr>
in both commits or the second one I added?
< sipa>
in all of them
< sipa>
do you understand what i mean?
< fjahr>
So there is already a variable $MERGE_BASE? I did not know that
< sipa>
you're using it!
< sipa>
COMMIT_RANGE="$MERGE_BASE..HEAD"
< sipa>
oh
< sipa>
i'm misreading, i see
< sipa>
ignore me
< fjahr>
:)
< sipa>
try COMMIT_RANGE="$TRAVIS_BRANCH..HEAD" ?
< sipa>
TRAVIS_BRANCH:
< sipa>
for push builds, or builds not triggered by a pull request, this is the name of the branch. for builds triggered by a pull request this is the name of the branch targeted by the pull request.
< sipa>
for builds triggered by a tag, this is the same as the name of the tag (TRAVIS_TAG).
< fjahr>
I thought it might be good to not be dependent on travis variables. But you think it will be more robust?
< fjahr>
Ah, you mean only in 06_script.sh, not the other places...
< sipa>
fjahr: i think your current code will cause non-master PRs to recheck the entire history back to where the release branch forked off
< fjahr>
ah, I see, I think that is right
< bitcoin-git>
[bitcoin] robot-dreams opened pull request #20074: test: p2p_blockfilters tests for BIP157 config args (master...bip157-blockfilters-tests) https://github.com/bitcoin/bitcoin/pull/20074
< bitcoin-git>
[bitcoin] sipa opened pull request #20075: [DO NOT MERGE] Version of #20071 rebased on buggy commit, with extra logging (master...pr20071) https://github.com/bitcoin/bitcoin/pull/20075
< bitcoin-git>
[bitcoin] sipa closed pull request #20075: [DO NOT MERGE] Version of #20071 rebased on buggy commit, with extra logging (master...pr20071) https://github.com/bitcoin/bitcoin/pull/20075
< dhruvm>
wumpus: Referring to your message from 08:57, what made me curious about these cli flag combinations is that I am new to the codebase and trying to estaablish a mental model for how the p2p layer works. For me this has involved going through the code, asking people I have access to questions about why things are a certain way.
< dhruvm>
When I saw that getaddr calls are still made in -connect mode I found myself confused as a new developer, and thought this may be confusing to future new devs as well. So the primary motivation was keeping things easy to understand.