Skip to content

p2p: always set nTime for self-advertisements #25314

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 21, 2022
Merged

p2p: always set nTime for self-advertisements #25314

merged 1 commit into from
Jun 21, 2022

Conversation

mzumsande
Copy link
Contributor

@mzumsande mzumsande commented Jun 8, 2022 •
edited
Loading

This logic was recently changed in 0cfc0cd to overwrite addrLocal with the address they gave us when self-advertising to an inbound peer. But if we don't also change nTime again from the default TIME_INIT, our peer will not relay our advertised address any further.

@mzumsande
Copy link
Contributor Author

ping @vasild

@fanquake fanquake added the P2P label Jun 8, 2022
@mzumsande mzumsande force-pushed the 202206_missing_ntime branch from 0857849 to 65749ee Compare June 8, 2022 21:45
@naumenkogs
Copy link
Member

utACK 65749ee
good catch. could be a good functional test?

@mzumsande
Copy link
Contributor Author

good catch. could be a good functional test?

I think that would be hard, because the local addresses in the functional tests are unroutable, so nodes don't self-advertise there. There is even the workaround of the -addrmantest parameter used in p2p_node_network_limited.py for this, but that just returns a fixed addr so the code changed here isn't executed.
However, the unit test net_tests/get_local_addr_for_peer_port covers this (and would fail without the update in this PR). Before, it would just check that the expected and actual CAddress have a default-initialized nTime.

vasild
vasild reviewed Jun 9, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

The proficiency of the engineers is just astonishing in this community (@mzumsande)! How the hell did you catch this?

@mzumsande
Copy link
Contributor Author

The proficiency of the engineers is just astonishing in this community (@mzumsande)! How the hell did you catch this?

Thank you! Pretty much by chance - I was reviewing #25303 and while looking at this commit msg I was wondering if it was possible to somehow have addresses with default-initialised nTime in addrman or addr relay  - that's how I stumbled upon this.

@mzumsande
p2p: always set nTime for self-advertisements
99b9e5f 
If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.
@mzumsande mzumsande force-pushed the 202206_missing_ntime branch from 65749ee to 99b9e5f Compare June 10, 2022 14:51
@mzumsande
Copy link
Contributor Author

65749ee -> 99b9e5f
Addressed feedback by @vasild (thanks!)

vasild
vasild approved these changes Jun 13, 2022
View reviewed changes
Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 99b9e5f

Thank you!

@naumenkogs
Copy link
Member

ACK 99b9e5f

@laanwj laanwj added this to the 24.0 milestone Jun 13, 2022
@fanquake fanquake requested a review from ajtowns June 13, 2022 11:22
@DrahtBot DrahtBot mentioned this pull request Jun 16, 2022
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 16, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

@laanwj
Copy link
Member

laanwj commented Jun 21, 2022

Code review ACK 99b9e5f
Great catch!

@laanwj laanwj merged commit c3a41ad into bitcoin:master Jun 21, 2022
@mzumsande
Copy link
Contributor Author

Maybe consider backporting to 23.x if this meets the bar.

@mzumsande mzumsande deleted the 202206_missing_ntime branch June 21, 2022 23:24
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 22, 2022
@mzumsande @fanquake
p2p: always set nTime for self-advertisements
4ebf6e3 
If we self-advertised to an inbound peer with the address they gave us,
nTime was left default-initialized, so that our peer wouldn't relay it
any further along.

Github-Pull: bitcoin#25314
Rebased-From: 99b9e5f
@fanquake
Copy link
Member

Maybe consider backporting to 23.x if this meets the bar.

I've added this for backport in #25316

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 22, 2022
@laanwj
Merge bitcoin#25314: p2p: always set nTime for self-advertisements
b63260e 
99b9e5f p2p: always set nTime for self-advertisements (Martin Zumsande)

Pull request description:

  This logic was recently changed in bitcoin@0cfc0cd to overwrite `addrLocal` with the address they gave us when self-advertising to an inbound peer. But if we don't also change `nTime` again from the default `TIME_INIT`, our peer will not relay our advertised address any further.

ACKs for top commit:
  naumenkogs:
    ACK 99b9e5f
  laanwj:
    Code review ACK 99b9e5f
  vasild:
    ACK 99b9e5f

Tree-SHA512: 4c7ea51cc77ddaa4b3537962ad2ad085f7ef5322982d3b1f5baecb852719eb99dd578436ca63432cb6b0a4fbd8b59fca793caf326c4663a4d6f34301e8146aa2
@fanquake fanquake mentioned this pull request Jun 22, 2022
Merged
@jnewbery jnewbery mentioned this pull request Jul 4, 2022
Merged
maflcko pushed a commit that referenced this pull request Jul 8, 2022
Merge #25316: 23.x backports
a33ec8a 
4ebf6e3 p2p: always set nTime for self-advertisements (Martin Zumsande)
039ef21 tests: Use descriptor that requires both legacy and segwit (Andrew Chow)
5fd25eb tests: Calculate input weight more accurately (Andrew Chow)
bd6d3ac windeploy: Renewed windows code signing certificate (Andrew Chow)
32fa522 test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg)
7658055 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg)

Pull request description:

  Backports:
  - #24454
  - #25201
  - #25220
  - #25314

ACKs for top commit:
  LarryRuane:
    re-utACK 4ebf6e3
  achow101:
    ACK 4ebf6e3

Tree-SHA512: add3999d0330b3442f3894fce38ad9b5adc75da7d681c949e1d052bac5520c2c6fb06eba98bfbeb4aa9a560170451d24bf00d08dddd4a3d080030ecb8ad61882
@bitcoin bitcoin locked and limited conversation to collaborators Jul 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ajtowns ajtowns Awaiting requested review from ajtowns

1 more reviewer

@vasild vasild vasild approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
P2P
Projects
None yet
Milestone
24.0
Development

Successfully merging this pull request may close these issues.
6 participants
@mzumsande @naumenkogs @DrahtBot @laanwj @fanquake @vasild