search

radicle-dev / radicle-link

The second iteration of the Radicle code collaboration protocol.
Other
423 stars 39 forks source link

daemon: Announce user updates #689

Closed CodeSandwich closed 3 years ago

CodeSandwich commented 3 years ago

This fixes an issue with users not seeing each others' updates, e.g. of Ethereum claims

CodeSandwich commented 3 years ago

@FintanH I've added this. Now it processes every URN received from any::list. It pushes both projects and persons through list_owner_project_refs, but all it seems to be doing is receiving the OIDs for a URN. Is this what you've suggested?

I'm not sure, but it looks like now the node will track all persons and projects it can lay eyes on. Is this necessary? After the addition of announcements of person updates my 2 instances of upstream on 2 machines started seeing their person updates. What's the reason to broaden the listening spectrum?

CodeSandwich commented 3 years ago

@xla

Extending the announcement subroutine is the more general way to solve this.

I don't follow, how should it be extended?

FintanH commented 3 years ago

It pushes both projects and persons through list_owner_project_refs, but all it seems to be doing is receiving the OIDs for a URN. Is this what you've suggested?

Ya, list_owner_project_refs is a bit of a misnomer really. It lists the signed_refs of any identity. So as you said it's listing the lastest Oid for a Urn. So what's happening is that when the annoucnement store notices a new change it will make the announcement and keep track of it in the store.

I'm not sure, but it looks like now the node will track all persons and projects it can lay eyes on. Is this necessary?

That's a question for the radicle-seed implementation. But note that it will track persons anyway if you create/replicate projects because your Person information is needed as part of rad/self and rad/ids/*.

FintanH commented 3 years ago

Ya, list_owner_project_refs is a bit of a misnomer really.

Could we rename this to load_refs instead, while we're here? :)

CodeSandwich commented 3 years ago

Renamed :)

xla commented 3 years ago

I see that init_project has one too, so maybe you were copying that? I'm not actually sure why that needs to be there. Could be an artifact of "UX". Do you know @xla?

That's an artifact from when the announcement wasn't running as frequently, smoke and mirrors. The explicit announce call can go.

CodeSandwich commented 3 years ago

Can we clean up and merge this PR, @FintanH? It's obviously fixing some issue, the test shows that, but it'll be much easier to work and look for the bugs after the merge. The constant rebasing and backporting is introducing massive complexity and uncertainty, which obscures the real problems and eats away hours of work.

FintanH commented 3 years ago

It's obviously fixing some issue, the test shows that, but it'll be much easier to work and look for the bugs after the merge.

Sorry, I don't understand your reasoning here. The test works without the explicit announce calls, so that means the bug you're witnessing is down to something else and the announcement loop is emitting the gossip correctly.

Understandable that it may be frustrating to rebase, and I'm happy for us to get this into a mergeable state. But I'd also like to understand what's your plan to find the bug then?

CodeSandwich commented 3 years ago

The test works without the explicit announce calls

You're right, the announces aren't needed, I've dropped them. But changes to build are fixing the test and IMO they are worth merging for that reason alone. That's probably our bug fixed or at least it's a step in the right direction. It's hard to tell if it fixes the issue completely, there may be additional steps, but it'll be easier to find out and keep track after the merge.

FintanH commented 3 years ago

Ya, the improvement of build should be merged! =] Sorry, maybe I've been communicating poorly here -- the explicit announce calls were what I wanted to prevent being introduced and I assumed that they were the "fix" to the upstream issue. I'll take one last pass and should be good to go

CodeSandwich commented 3 years ago

@xla How about you? This PR is blocked unless you approve it.

xla commented 3 years ago

CI is not running all the way through to the tests because of an issue in a dependency. Can you reproduce locally?

CodeSandwich commented 3 years ago

@xla It's compiling fine locally. All the CI runs are failing for the same reason and it's the same in all currently open PRs.

xla commented 3 years ago

@CodeSandwich Could you merge/rebase master to have the tests run properlier on CI.