service/block, node/p2p: Fix race conditions in TestExtendedHeaderBroadcast and TestFull_P2P_Streams

[ynewmann]

PR supposed to:

• fix [Bug]: Race Condition in TestFull_P2P_Streams #286;
• fix [Bug]: Race Condition in TestExtendedHeaderBroadcast #283;
• and maybe fix panic: AddrFactory #232

[Bidon15]

Thank you so much for the PR @jenyasd209 ! 🥳
It would be great if you could:

1. Update the changelog.md
2. Fix the lint error on maAnnounce[:]
3. Take a look on new error thrown during failure of TestExtendedHeaderBroadcast

[ynewmann]

3. Take a look on new error thrown during failure of TestExtendedHeaderBroadcast

Updated. The problem is I have no failures locally (go v1.16)

Update: checked locally with go v1.17 and still have no issues. As I can see the error is context deadline exceeded, for some reasons 15 seconds weren't enough for the test. Does the test need strictly 15 seconds?

[Wondertan]

@jenyasd209, thank you for taking over bugs!

I am fine with the Race fixes, but unsure about #232 being fixed. The bug described there is not reproducible and happened only once in an unusual situation. Furthermore, I am not sure why this bag happened in the first place. There should not be any panics as append(on the line 50) should not produce such errors, due to the way appends works. Also, a lot of things changed from the point the bug appeared.

Let's proceed by removing your fix for #232 in this PR and closing the issue, as there should be no panics on the code as it is right now and the fix does not really fix anything.

[Wondertan]

@jenyasd209, also, the error you were facing is test flakiness and can be resolved by re-running all tests using github actions page

[ynewmann]

@jenyasd209, thank you for taking over bugs!

I am fine with the Race fixes, but unsure about #232 being fixed. The bug described there is not reproducible and happened only once in an unusual situation. Furthermore, I am not sure why this bag happened in the first place. There should not be any panics as append(on the line 50) should not produce such errors, due to the way appends works. Also, a lot of things changed from the point the bug appeared.

Let's proceed by removing your fix for #232 in this PR and closing the issue, as there should be no panics on the code as it is right now and the fix does not really fix anything.

I tagged #232 because it looks similar to #286 (logs https://gist.github.com/Bidon15/9ec686472ed21ee77662fca18dc5ecf2). There is race condition at the same line where panic happens. Due to append is not thread safe panic can happen if there are a couple calls of this func.

[Wondertan]

@jenyasd209, ooh, I see. Thanks for pointing me to the connection with the races. I made a deeper look and there is clearly a bug/typo. We weren't even using maAnnounce(as you pointed in #288 (comment)) after the append, as you can see there. We should append there not to the maAnnounce, but to maListen, i.e: maListen = append(maListen, maAnnounce...). At the first sight, I was even thinking that this happens. Also, this resolves races, as no more appending of the shared slice will happen.

[Wondertan]

Awesome, thank you!

[renaynay]

Small nit but otherwise good. Unfortunately TestExtendedHeaderBroadcast will no longer be needed as header production will be removed from BlockService anyway with #277 .

[Wondertan]

@jenyasd209, can you pls make a rebase?