p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty #19884

dhruv · 2020-09-05T16:46:06Z

Closes #19795

Before PR: If peers.dat is empty and -dnsseed=0, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay.
After PR: There's no 60 second delay.

To reproduce:
rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0 without and with patch code

Other changes in the PR:

-fixedseeds command line argument added: -dnsseed=0 -fixedseeds=0 -addnode=X provides a trusted peer only setup. -dnsseed=0 -fixedseeds=0 allows for a addnode RPC to add a trusted peer without falling back to hardcoded seeds.

n-thumann

tACK bcbd637 on master at 3ba25e3, was able to reproduce the issue and confirm that it´s fixed with this PR ✌️

dhruv · 2020-09-05T17:23:43Z

@practicalswift I'm sorry this took so long. Ready for review.

dhruv · 2020-09-05T17:24:37Z

@n-thumann Thank you, for the prompt review.

src/net.cpp

jonatack

Concept ACK. Maybe add a regression test in feature_config_args.py to cover these cases.

src/net.cpp

practicalswift · 2020-09-05T18:49:58Z

@dhruv I saw you removed the done logic: are you sure that !addrman.empty() is guaranteed after addrman.Add(convertSeed6(Params().FixedSeeds()), local);? AFAICT that condition must be hold for the new code to be equivalent to the old code.

dhruv

@jonatack Added a regression test in feature_config_args.py to cover the cases. It's a slow test though because we have to wait 60 seconds. Is there anything I can do to reduce that interval for tests? I thought about making it configurable via cli flags but I will look to your guidance on whether that is standard practice.

src/net.cpp

dhruv · 2020-09-06T00:37:35Z

@dhruv I saw you removed the done logic: are you sure that !addrman.empty() is guaranteed after addrman.Add(convertSeed6(Params().FixedSeeds()), local);? AFAICT that condition must be hold for the new code to be equivalent to the old code.

@practicalswift Thank you for catching that. I overlooked the implication of the static qualifier on done and mistakenly thought the code was redundant. It is now added back in.

It seems that the done logic is used to make sure that addrman.Add(convertSeed6(Params().FixedSeeds()), local); gets called no more than once regardless of the outcome. When I followed the code in CAddrMan, it seems that the invocation just adds the fixed seeds to the address tables so long as the seeds are well-formed. So invoking the deterministic function more than once is unnecessary. I'm new around here, so I'm hoping you can confirm my understanding.

dhruv · 2020-09-07T05:38:43Z

Appveyor has passed on e7de5b5, Github is just not reflecting it.

jonatack

ACK e7de5b5

Thanks for adding the tests. No strong opinion whether it's better to drop the slow first test or keep it. There is a setmocktime RPC often used in the functional tests, and a -mocktime=<n> config option that could be passed an extra_arg to start_node, but AFAICT neither one will have any effect and I'm not sure it's worth changing the code to be more testable unless you have an elegant way to do it or it could be generalized and useful elsewhere.

A couple of nit comments follow; you can ignore them unless you need to retouch for other reasons.

src/net.cpp

test/functional/feature_config_args.py

ajtowns · 2020-09-10T23:24:13Z

If peers.dat is empty and -dnsseed=0

Is this a use case worth optimising for? The linked issue doesn't really provide a reason why this use case is worthwhile, as far as I can see, just that the filer thought it was surprising.

The only case I can see where you'd want to set -dnsseed=0 is if you somehow don't have DNS despite being connected to the real internet, or if you're really worried about privacy. But if you're really worried about privacy, connecting to fixed seeds is probably worse than doing a dns query, and if you were doing something clever, like triggering an outbound/inbound connection to your own node, losing the 60 second delay might also drop privacy.

dhruv · 2020-09-11T04:20:31Z

Is this a use case worth optimising for? The linked issue doesn't really provide a reason why this use case is worthwhile, as far as I can see, just that the filer thought it was surprising.

Fair question, @ajtowns. Let's page @practicalswift and see if he can shed light on his original use case.

practicalswift · 2020-09-11T06:01:13Z

The only case I can see where you'd want to set -dnsseed=0 is if you somehow don't have DNS despite being connected to the real internet, or if you're really worried about privacy. But if you're really worried about privacy, connecting to fixed seeds is probably worse than doing a dns query, and if you were doing something clever, like triggering an outbound/inbound connection to your own node, losing the 60 second delay might also drop privacy.

There are multiple important use cases for -dnsseed=0:

You don't want to use clearnet at all (-dnsseed=0 -onlynet=onion -listen=0)
You don't want to broadcast to the DNS seed operators (or anyone listening on the path to those DNS seed operators) that you're starting a new node (-dnsseed=0 -listen=0)
You want to test how Bitcoin Core behaves if the DNS seeds are unavailable

I hope you're not advocating the removal of -dnsseed=0 :)

Is this a use case worth optimising for? The linked issue doesn't really provide a reason why this use case is worthwhile, as far as I can see, just that the filer thought it was surprising.

I'm not sure I would call removal of a 60 second sleep an optimization. To me it is clearly a bug fix :)

Also, please note that the behaviour is unchanged if -dnsseed=0 is not set.

The only effect of this change is that users of -dnsseed=0 won't have to wait an extra 60 seconds for their node to start.

Given the above clarification: do your skepticism to this bug fix remain? :)

sipa · 2020-09-11T06:16:35Z

You don't want to use clearnet at all (-dnsseed=0 -onlynet=onion -listen=0)

FWIW, if you use -proxy, DNS seeding happens over the proxy (which in the case of Tor means the exit node will do the resolving for you).

practicalswift · 2020-09-11T06:21:07Z

You don't want to use clearnet at all (-dnsseed=0 -onlynet=onion -listen=0)

FWIW, if you use -proxy, DNS seeding happens over the proxy (which in the case of Tor means the exit node will do the resolving for you).

Yes I know, but then you'll have to trust the exit node operator not to tamper with the DNS responses :)

-dnsseed=0 -onlynet=onion -listen=0 avoids that.

sipa · 2020-09-11T06:23:15Z

Of course, and I'm not commenting on the validity of the use case in general. Just that "You don't want to use clearnet at all" on its own is not a reason to use -dnsseed=0.

practicalswift · 2020-09-11T06:25:14Z

Of course, and I'm not commenting on the validity of the use case in general. Just that "You don't want to use clearnet at all" on its own is not a reason to use -dnsseed=0.

Fair point. I should have written "You don't want to use clearnet at all and you don't trust exit node operators" :)

practicalswift · 2020-09-11T09:07:55Z

FWIW, if you use -proxy, DNS seeding happens over the proxy (which in the case of Tor means the exit node will do the resolving for you).

By "DNS seeding" you're in this context referring to the "establish ADDR_FETCH connections via proxy to seed nodes"-logic, right?

AFAICT we've never supported sending actual DNS queries to the DNS seed nodes via proxy.

From the SetNameProxy documentation: "SOCKS5's support for UDP-over-SOCKS5 has been considered, but no SOCK5 server in common use (most notably Tor) actually implements UDP support, and a DNS resolver is beyond the scope of this project."

practicalswift

Tested ACK e7de5b5

This fixes the bug which makes -dnsseed=0 users have to wait out a 60 second sleep intended only for -dnsseed=1 users :)

sipa · 2020-09-11T17:20:33Z

@practicalswift Yes, when -proxy is enabled, no actual DNS seeding is done, but ADDR_FETCH connections are established to the seeder's names instead. That was the reason for introducing ADDR_FETCH connections (formerly called oneshot connections) initially.

ajtowns · 2020-09-12T22:20:21Z

Fair point. I should have written "You don't want to use clearnet at all and you don't trust exit node operators" :)

Ah, and are thus only connecting to the ~89 fixed seeds with onion addresses, and not going via exit nodes at all? That makes more sense.

I think that currently, if you do -onlynet=onion -listen=0 -dnsseed=0 -addnode=mytrustedpeer.onion, and you successfully connect to mytrustedpeer.onion within 60 seconds, you won't add the fixed seeds (your addrman will contain mypeer.onion rather than being empty), whereas with this change you will add the fixed seeds. Not sure that's a change for the better?

Perhaps rather than a time check, it would be better to check for something like "if addrman is empty, there are no ADDR_FETCH nodes left to try, no open connections and no added nodes to keep trying" ?

OTOH, if it is a change for the better, it might make sense to move the add fixed nodes code to its own function, and call it from ThreadDNSAddressSeed directly (after trying all the dns seeds, with addrman still empty, and only after the opencon thread as processed any ADDR_FETCH nodes that got queued up), and from CConnman::Start when -dnsseed=0 and addrman is empty. That way you avoid unnecessary waiting in all cases, I think.

(edit: ADDR_FETCH not oneshot)

dhruv · 2020-12-11T15:57:04Z

Rebased

LarryRuane

As suggested in the description, I tested:

rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0

without the patch (note the timestamps):

2021-01-02T00:00:49Z msghand thread start
2021-01-02T00:01:50Z Adding fixed seed nodes as DNS doesn't seem to be available.

With the patch:

2021-01-02T00:05:38Z msghand thread start
2021-01-02T00:05:38Z Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted

Ran clang-format-diff.py (it found nothing). Nice PR! Suggestions are minor and optional.
ACK eb1eef4
(but take with a grain of salt; I don't know this area of the code well.)

src/init.cpp

src/net.cpp

test/functional/feature_config_args.py

LarryRuane · 2021-01-05T01:23:51Z

test/functional/feature_config_args.py

@@ -145,11 +146,61 @@ def test_networkactive(self):
            self.start_node(0, extra_args=['-nonetworkactive=1'])
        self.stop_node(0)

+    def test_seed_peers(self):
+        self.log.info('Test seed peers')


Suggested change

self.log.info('Test seed peers')

self.log.info('Test seed peers, this will take about 2 minutes')

LarryRuane · 2021-01-05T01:27:20Z

test/functional/feature_config_args.py

+        with self.nodes[0].assert_debug_log(expected_msgs=["Loaded 0 addresses from peers.dat",
+                "DNS seeding disabled",
+                "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"]):


You can also check for the absence of debug.log messages (I'm not sure if it's worth it in this case):

Suggested change

with self.nodes[0].assert_debug_log(expected_msgs=["Loaded 0 addresses from peers.dat",

"DNS seeding disabled",

"Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"]):

with self.nodes[0].assert_debug_log(expected_msgs=[

"Loaded 0 addresses from peers.dat",

"DNS seeding disabled",

"Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"

], unexpected_msgs=["60 seconds"]):

Changed the indentation. I did not add the unexpected_msgs=["60 seconds"] as it is implicit in the 2 second timeout for assert_debug_log.

dhruv · 2021-02-06T01:36:37Z

Thanks for the review @LarryRuane, and my apologies for the long wait.

Comments addressed. Rebased. Ready for further review.

dhruv · 2021-02-06T01:46:58Z

Fixed failing linter.

LarryRuane · 2021-02-11T02:39:04Z

re-reviewed, re-tested
ACK 3fdea1f

laanwj · 2021-02-11T18:23:09Z

Code review ACK 3fdea1f

laanwj · 2021-02-11T18:25:48Z

test/functional/feature_config_args.py

+                "DNS seeding disabled",
+                "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"]):
+            self.start_node(0, extra_args=['-dnsseed=0'])
+        assert time.time() - start < 5


It's slightly risky to have a test that depends on real time instead of mocked times. CI environments can be extremely slow, so this bound might introduce random failures.

You're right - that could make for a flaky test. I picked 5 seconds as an arbitrary bound. What I really wanted to check was that the node did not wait 60 seconds to use fixed seeds.

Suggested change

assert time.time() - start < 5

assert time.time() - start < 60

accomplished that. And if CI is so slow that 60 seconds pass, the test will fail anyway because the logs will say "Adding fixed seeds as 60 seconds have passed and addrman is empty" instead of "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted".

Thanks, good idea!

In the longer term I think we should change this to use mock time but for this PR that would be too much of a hassle.

…mpty. Add -fixedseeds arg.

LarryRuane · 2021-02-12T00:23:51Z

re-ACK fe3e993

dhruv · 2021-02-12T02:42:16Z

Updated tests for CI stability. Ready for further review.

laanwj · 2021-02-12T10:48:03Z

re-ACK fe3e993
checked that the only change since my last ACK is the < time bounds for the test

…0 and peers.dat is empty fe3e993 [p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty. Add -fixedseeds arg. (Dhruv Mehta) Pull request description: Closes bitcoin#19795 Before PR: If `peers.dat` is empty and `-dnsseed=0`, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay. After PR: There's no 60 second delay. To reproduce: `rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0` without and with patch code Other changes in the PR: - `-fixedseeds` command line argument added: `-dnsseed=0 -fixedseeds=0 -addnode=X` provides a trusted peer only setup. `-dnsseed=0 -fixedseeds=0` allows for a `addnode` RPC to add a trusted peer without falling back to hardcoded seeds. ACKs for top commit: LarryRuane: re-ACK fe3e993 laanwj: re-ACK fe3e993 Tree-SHA512: 79449bf4e83a315be6dbac9bdd226de89d2a3f7f76d9c5640a2cb3572866e6b0e8ed67e65674c9824054cf13119dc01c7e1a33848daac6b6c34dbc158b6dba8f

maflcko · 2021-02-12T16:13:37Z

test/functional/feature_config_args.py

+        with self.nodes[0].assert_debug_log(expected_msgs=[
+                "Loaded 0 addresses from peers.dat",
+                "DNS seeding disabled",
+                "Adding fixed seeds as -dnsseed=0, -addnode is not provided and and all -seednode(s) attempted\n"]):


Please see #21165

maflcko · 2021-02-12T16:14:43Z

test/functional/feature_config_args.py

+                "0 addresses found from DNS seeds",
+                "Adding fixed seeds as 60 seconds have passed and addrman is empty"], timeout=80):
+            self.start_node(0, extra_args=['-dnsseed=1'])
+        assert time.time() - start >= 60


wouldn't it work to use setmocktime inside the context manager above and check here: node.uptime() >= 60 to speed up the test?

I looked at this last September (#19884 (review)) and it didn't look feasible, interesting if it is.

$ time test/functional/feature_config_args.py 2021-02-12T17:02:39.647000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_kuphxowf 2021-02-12T17:02:45.294000Z TestFramework (INFO): Test config args logging 2021-02-12T17:02:45.605000Z TestFramework (INFO): Test seed peers, this will take about 2 minutes 2021-02-12T17:02:48.587000Z TestFramework (INFO): Test -networkactive option 2021-02-12T17:02:54.023000Z TestFramework (INFO): Stopping nodes 2021-02-12T17:02:54.127000Z TestFramework (INFO): Cleaning up /tmp/bitcoin_func_test_kuphxowf on exit 2021-02-12T17:02:54.127000Z TestFramework (INFO): Tests successful real 0m14.682s user 0m3.842s sys 0m0.745s

Please see #21165

fa730e9 test: Avoid connecting to real network when running tests (MarcoFalke) fa1b713 test: Assume node is running in subtests (MarcoFalke) Pull request description: Introduced in #19884 ACKs for top commit: Sjors: ACK fa730e9 Tree-SHA512: fe132a9ffe2fae1ab16857a3dec9839526fdf74d27a1ae794fbffca8356f639c4b916dc888b260281e9cc793916706c18d1687ebb5a076d4e1c481d218d308d3

…ing tests fa730e9 test: Avoid connecting to real network when running tests (MarcoFalke) fa1b713 test: Assume node is running in subtests (MarcoFalke) Pull request description: Introduced in bitcoin#19884 ACKs for top commit: Sjors: ACK fa730e9 Tree-SHA512: fe132a9ffe2fae1ab16857a3dec9839526fdf74d27a1ae794fbffca8356f639c4b916dc888b260281e9cc793916706c18d1687ebb5a076d4e1c481d218d308d3

…mpty. Add -fixedseeds arg. Github-Pull: bitcoin#19884 Rebased-From: fe3e993

…mpty. Add -fixedseeds arg. Summary: ``` Before PR: If peers.dat is empty and -dnsseed=0, bitcoind will fallback on to fixed seeds but only after a 60 seconds delay. After PR: There's no 60 second delay. To reproduce: rm ~/.bitcoin/peers.dat && src/bitcoind -dnsseed=0 without and with patch code Other changes in the PR: -fixedseeds command line argument added: -dnsseed=0 -fixedseeds=0 -addnode=X provides a trusted peer only setup. -dnsseed=0 -fixedseeds=0 allows for a addnode RPC to add a trusted peer without falling back to hardcoded seeds. ``` Note to reviewers 1: the p2p_dos_header_tree.py test change is cherry picked from bitcoin/bitcoin@fa730e9. Without this the test will connect to the real testnet and eventually sync the headers with other nodes, that would cause the test to fail. This will be undone when the PR is backported, which requires D10909 as a dependency. Note to reviewers 2: the test feature_config_args.py now takes forever to run, this will be fixed in a follow-up. Backport of [[bitcoin/bitcoin#19884 | core#19884]]. Depends on D10907. Ref T1696. Test Plan: ./test/functional/test_runner.py feature_config_args.py Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Subscribers: PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10908

dhruv force-pushed the no-delay-fixed-peer-seeds branch from efdbbff to bcbd637 Compare September 5, 2020 16:52

dhruv changed the title ~~No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty~~ p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty Sep 5, 2020

n-thumann approved these changes Sep 5, 2020

View reviewed changes

DrahtBot added the P2P label Sep 5, 2020

practicalswift reviewed Sep 5, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Sep 5, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

dhruv force-pushed the no-delay-fixed-peer-seeds branch from bcbd637 to ce0686c Compare September 5, 2020 18:10

jonatack reviewed Sep 5, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

dhruv force-pushed the no-delay-fixed-peer-seeds branch from ce0686c to 34c215c Compare September 6, 2020 00:20

dhruv commented Sep 6, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

dhruv force-pushed the no-delay-fixed-peer-seeds branch from 34c215c to e7de5b5 Compare September 6, 2020 00:41

dhruv requested review from jonatack and practicalswift September 10, 2020 14:38

jonatack reviewed Sep 10, 2020

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

test/functional/feature_config_args.py Outdated Show resolved Hide resolved

practicalswift approved these changes Sep 11, 2020

View reviewed changes

dhruv force-pushed the no-delay-fixed-peer-seeds branch from c68eef3 to aaf9cae Compare November 28, 2020 20:41

DrahtBot added the Needs rebase label Dec 11, 2020

dhruv force-pushed the no-delay-fixed-peer-seeds branch from aaf9cae to eb1eef4 Compare December 11, 2020 15:55

DrahtBot removed the Needs rebase label Dec 11, 2020

DrahtBot mentioned this pull request Dec 20, 2020

p2p: standardize outbound full/block relay connection type naming #20729

Closed

LarryRuane reviewed Jan 5, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 27, 2021

Make all of net_processing (and some of net) use std::chrono types #21015

Merged

dhruv force-pushed the no-delay-fixed-peer-seeds branch from eb1eef4 to 500315d Compare February 6, 2021 01:31

dhruv force-pushed the no-delay-fixed-peer-seeds branch from 500315d to 3fdea1f Compare February 6, 2021 01:42

laanwj reviewed Feb 11, 2021

View reviewed changes

[p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is e…

fe3e993

…mpty. Add -fixedseeds arg.

dhruv force-pushed the no-delay-fixed-peer-seeds branch from 3fdea1f to fe3e993 Compare February 12, 2021 00:11

laanwj merged commit e9c037b into bitcoin:master Feb 12, 2021

maflcko reviewed Feb 12, 2021

View reviewed changes

dhruv mentioned this pull request Feb 12, 2021

test: Use mocktime in test_seed_peers #21165

Merged

maflcko mentioned this pull request Feb 21, 2021

test: Avoid connecting to real network when running tests #21254

Merged

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 27, 2021

[p2p] No delay in adding fixed seeds if -dnsseed=0 and peers.dat is e…

0169da9

…mpty. Add -fixedseeds arg. Github-Pull: bitcoin#19884 Rebased-From: fe3e993

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

	self.log.info('Test seed peers')
	self.log.info('Test seed peers, this will take about 2 minutes')

	assert time.time() - start < 5
	assert time.time() - start < 60

p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty #19884

p2p: No delay in adding fixed seeds if -dnsseed=0 and peers.dat is empty #19884

Conversation

dhruv commented Sep 5, 2020 • edited

n-thumann left a comment • edited

Choose a reason for hiding this comment

dhruv commented Sep 5, 2020

dhruv commented Sep 5, 2020

jonatack left a comment

Choose a reason for hiding this comment

practicalswift commented Sep 5, 2020 • edited

dhruv left a comment

Choose a reason for hiding this comment

dhruv commented Sep 6, 2020

dhruv commented Sep 7, 2020

jonatack left a comment • edited

Choose a reason for hiding this comment

ajtowns commented Sep 10, 2020

dhruv commented Sep 11, 2020

practicalswift commented Sep 11, 2020 • edited

sipa commented Sep 11, 2020 • edited

practicalswift commented Sep 11, 2020 • edited

sipa commented Sep 11, 2020

practicalswift commented Sep 11, 2020

practicalswift commented Sep 11, 2020 • edited

practicalswift left a comment • edited

Choose a reason for hiding this comment

sipa commented Sep 11, 2020

ajtowns commented Sep 12, 2020 • edited

dhruv commented Dec 11, 2020

LarryRuane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruv commented Feb 6, 2021

dhruv commented Feb 6, 2021

LarryRuane commented Feb 11, 2021

laanwj commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laanwj Feb 12, 2021 • edited

Choose a reason for hiding this comment

LarryRuane commented Feb 12, 2021

dhruv commented Feb 12, 2021

laanwj commented Feb 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhruv commented Sep 5, 2020 •

edited

n-thumann left a comment •

edited

practicalswift commented Sep 5, 2020 •

edited

jonatack left a comment •

edited

practicalswift commented Sep 11, 2020 •

edited

sipa commented Sep 11, 2020 •

edited

practicalswift commented Sep 11, 2020 •

edited

practicalswift commented Sep 11, 2020 •

edited

practicalswift left a comment •

edited

ajtowns commented Sep 12, 2020 •

edited

laanwj Feb 12, 2021 •

edited