[WIP] Clarify variable names #8622

rebroad commented at 5:31 AM on August 29, 2016: contributor

This code causes no functional difference to bitcoin.

The purpose of the pull is also to help make the code more readable by making variable names fit their function.

The first commit moves the debug message for version number to before the disconnect logic so that the debug.log will show the version of nodes connected to, however briefly. This seems reasonable given that a version message is received - it now ensures it is logged.
The second commit changed nServicesExpected so that it is what is in the address database, thereby making the variable function closer to the variable name.

rebroad closed this on Aug 29, 2016

rebroad reopened this on Aug 29, 2016

rebroad force-pushed on Aug 29, 2016

rebroad closed this on Aug 29, 2016

rebroad reopened this on Aug 29, 2016

rebroad force-pushed on Aug 29, 2016

rebroad closed this on Aug 29, 2016

rebroad commented at 7:27 AM on August 29, 2016: contributor

sorry... still needs testing

paveljanik commented at 7:33 AM on August 29, 2016: contributor

Can you please test locally first?

Display version message before disconnection criteria (no functional changes) 25c54b3d79

nServicesExpected now represents Services Expected rather than a subset. 817c19c6cb

rebroad commented at 8:08 AM on August 29, 2016: contributor

@paveljanik I do.. but not enough sometimes. I'm a premature pusher it seems :-s

Also, as soon as I am happy with my code, I want to push it to my github account, but if I do that while the pull is closed, then I'm unable to re-open it, so I have to open it before pushing. But if I leave it open, other developers on github complain if it's not read enough, which sometime it isn't, so I close it until I am sure.

If there is a better way to do this, please let me know. The opening and closing seems to be due to a feature in github which locks the pull closed if pushes are done while it is closed :-s

rebroad reopened this on Aug 29, 2016

rebroad force-pushed on Aug 29, 2016

rebroad closed this on Aug 29, 2016

MarcoFalke commented at 8:13 AM on August 29, 2016: member

@rebroad Leave it open or leave it closed. You already included the [WIP] tag.

Otherwise you are sending out about 1200 emails every time you click the button.

rebroad commented at 8:16 AM on August 29, 2016: contributor

@MarcoFalke ok, I will find a better way to do this. Does changing the title also send as many emails? I don't even feel having conversation via comments is the best way either - is there no private messaging function on github for example?

sipa commented at 8:21 AM on August 29, 2016: member

Pushing a commit to a PR, changing the title, opening, closing, ... all send mails. And people who are subscribed to the project receive all these mails from all pull requests.

If you want to actively experiment, why not work in a separate branch, without a PR? Once you're happy with the result, you can push the PR'ed branch.

btcdrak commented at 10:00 AM on August 29, 2016: contributor

@rebroad I also suggest popping into IRC to ask questions/sound ideas. It's a lot more efficient to get direction and focus.

in src/main.cpp:None in 817c19c6cb

4967 | +                  pfrom->nStartingHeight, addrMe.ToString(), pfrom->id,
4968 | +                  remoteAddr);
4969 | +
4970 | +        if (pfrom->nServicesExpected != pfrom->nServices) {
4971 | +            LogPrint("net", "peer=%d does not offer the expected services (%08x offered, %08x expected)", pfrom->id, pfrom->nServices, pfrom->nServicesExpected);
4972 | +            if (pfrom->nServicesExpected & nRelevantServices & ~pfrom->nServices) {

rebroad commented at 7:09 AM on September 1, 2016:

It seems to me that nServicesExpected doesn't need to be mentioned here at all. It's irrelevant. It's simply what was advertised in the addresses messages. If nServices includes all the relevant services then surely the connection should remain. Firstly, the network selection code is such that if the relevant services wasn't present in ExpectedServices it wouldn't even have tried connecting to it. and secondly, even if the network selection code tries connecting to it, then as long as nServices (the latest update of what the node provides) includes the relevant services, the connection should remain even if the ExpectedServices (an out of date snapshot of what other nodes say the node provides) says it doesn't provide the service.

I therefore propose changing this line to:- if (nRelevantServices & ~pfrom->nServices) {

any objections?

rebroad reopened this on Sep 1, 2016

sipa commented at 7:44 AM on September 1, 2016: member

I don't understand what you're trying to fix here.

sipa commented at 8:02 AM on September 1, 2016: member

No, as mentioned elsewhere, we definitely can connect to nodes that do not advertize all relevant service bits. Please stop trying find ways to change this before understanding the intention.

sipa closed this on Sep 1, 2016

DrahtBot locked this on Sep 8, 2021