💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1684075929)
[30b0f57 ](https://github.com/bitcoin/bitcoin/commit/30b0f57b19eddd47fa137d0c20ae39174b54dbad)to [17028c1](https://github.com/bitcoin/bitcoin/commit/17028c1fec3f109b139e5e23f535c86f5a1ac2a4):
Rebased, expanded log entry with `this might take up to 2 minutes`
(https://github.com/bitcoin/bitcoin/pull/26114#issuecomment-1684075929)
[30b0f57 ](https://github.com/bitcoin/bitcoin/commit/30b0f57b19eddd47fa137d0c20ae39174b54dbad)to [17028c1](https://github.com/bitcoin/bitcoin/commit/17028c1fec3f109b139e5e23f535c86f5a1ac2a4):
Rebased, expanded log entry with `this might take up to 2 minutes`
💬 mzumsande commented on pull request "net: Make AddrFetch connections to fixed seeds":
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1298579606)
added the log.
(https://github.com/bitcoin/bitcoin/pull/26114#discussion_r1298579606)
added the log.
⚠️ Sjors opened an issue: "macOS poll() tracking issue"
(https://github.com/bitcoin/bitcoin/issues/28297)
Since #14336 we use `poll()` on systems which support it properly and fall back to `select()` otherwise.
@jamesob did some research on the implementation used on macOS and [found it lacking](https://github.com/bitcoin/bitcoin/pull/14336#issuecomment-437384408). Although in principle our current use of it shouldn't cause a problem.
I checked to see if perhaps Apple has improved things since 2018. They moved the repo, so the `knote_fdfind` implementation is now here: https://github.com/apple
...
(https://github.com/bitcoin/bitcoin/issues/28297)
Since #14336 we use `poll()` on systems which support it properly and fall back to `select()` otherwise.
@jamesob did some research on the implementation used on macOS and [found it lacking](https://github.com/bitcoin/bitcoin/pull/14336#issuecomment-437384408). Although in principle our current use of it shouldn't cause a problem.
I checked to see if perhaps Apple has improved things since 2018. They moved the repo, so the `knote_fdfind` implementation is now here: https://github.com/apple
...
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1298590709)
Added
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1298590709)
Added
💬 fanquake commented on issue "macOS poll() tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28297#issuecomment-1684090201)
> In other words, we should probably leave things as they are for now,
So no need for an issue then? This is already tracked in the code.
(https://github.com/bitcoin/bitcoin/issues/28297#issuecomment-1684090201)
> In other words, we should probably leave things as they are for now,
So no need for an issue then? This is already tracked in the code.
👋 achow101's pull request is ready for review: "wallet: Use CTxDestination in CRecipient instead of just scriptPubKey"
(https://github.com/bitcoin/bitcoin/pull/28246)
(https://github.com/bitcoin/bitcoin/pull/28246)
💬 darosior commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1684100400)
> > Seems like this could result in banning pre-Segwit and pre-Taproot nodes?
>
> It would only impact nodes that produce/relay invalid segwit/taproot spends, ie ones that don't implement standardness checks. It also only triggers discouragement not banning, so affected peers can still reconnect. If we don't want to do that, we should probably drop `MaybePunishNodeForTx` entirely.
An invalid p2sh-wrapped Segwit spend [would be considered standard](https://github.com/bitcoin/bitcoin/blob/97
...
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1684100400)
> > Seems like this could result in banning pre-Segwit and pre-Taproot nodes?
>
> It would only impact nodes that produce/relay invalid segwit/taproot spends, ie ones that don't implement standardness checks. It also only triggers discouragement not banning, so affected peers can still reconnect. If we don't want to do that, we should probably drop `MaybePunishNodeForTx` entirely.
An invalid p2sh-wrapped Segwit spend [would be considered standard](https://github.com/bitcoin/bitcoin/blob/97
...
💬 Sjors commented on pull request "Fix potential network stalling bug":
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1684100848)
Post merge concept ACK. From my (very) limited understanding of sockets, this makes sense. Thanks @ajtowns for the description https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644977188.
(https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1684100848)
Post merge concept ACK. From my (very) limited understanding of sockets, this makes sense. Thanks @ajtowns for the description https://github.com/bitcoin/bitcoin/pull/27981#issuecomment-1644977188.
💬 Sjors commented on issue "macOS poll() tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28297#issuecomment-1684102520)
The code currently refers to a locked pull request where nobody can comment on progress. If you unlock it, then closing this one should be fine.
(https://github.com/bitcoin/bitcoin/issues/28297#issuecomment-1684102520)
The code currently refers to a locked pull request where nobody can comment on progress. If you unlock it, then closing this one should be fine.
💬 Sjors commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1684108390)
Should we add exception(s) to `MaybePunishNodeForTx` for where a soft-fork made a previously standard transaction consensus invalid? Is this the only time that happened?
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1684108390)
Should we add exception(s) to `MaybePunishNodeForTx` for where a soft-fork made a previously standard transaction consensus invalid? Is this the only time that happened?
💬 Sjors commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1684124388)
> * (sorry @Sjors, the earlier rebase was incorrect, it only looked at `vSendMsg.empty()` for the returned `data_left` in `SocketSendData()`).
Is there a test you can add that would catch this difference?
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1684124388)
> * (sorry @Sjors, the earlier rebase was incorrect, it only looked at `vSendMsg.empty()` for the returned `data_left` in `SocketSendData()`).
Is there a test you can add that would catch this difference?
💬 darosior commented on pull request "Update MANDATORY_SCRIPT_VERIFY_FLAGS":
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1684130991)
It's not really about `MaybePunishNodeForTx` (which already does not add a `Misbehaving` score for `TX_RECENT_CONSENSUS_CHANGE`), but rather to actually detect it's due to a recent consensus change and not a pre-existing consensus rule and return `TX_RECENT_CONSENSUS_CHANGE` in this case.
(https://github.com/bitcoin/bitcoin/pull/26291#issuecomment-1684130991)
It's not really about `MaybePunishNodeForTx` (which already does not add a `Misbehaving` score for `TX_RECENT_CONSENSUS_CHANGE`), but rather to actually detect it's due to a recent consensus change and not a pre-existing consensus rule and return `TX_RECENT_CONSENSUS_CHANGE` in this case.
💬 fanquake commented on issue "macOS poll() tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28297#issuecomment-1684145438)
I'd suggest updating the code with any relevant details, if they are missing. There shouldn't be a reason to comment on 5 year old Pull Request to "track" things.
(https://github.com/bitcoin/bitcoin/issues/28297#issuecomment-1684145438)
I'd suggest updating the code with any relevant details, if they are missing. There shouldn't be a reason to comment on 5 year old Pull Request to "track" things.
💬 MarcoFalke commented on pull request "ci: Switch more tasks to self-hosted":
(https://github.com/bitcoin/bitcoin/pull/21652#issuecomment-1684145978)
Added a description to the commits, to make it easier to review.
(https://github.com/bitcoin/bitcoin/pull/21652#issuecomment-1684145978)
Added a description to the commits, to make it easier to review.
💬 murchandamus commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1684150658)
Rebased to get rid of CI issues.
(https://github.com/bitcoin/bitcoin/pull/26152#issuecomment-1684150658)
Rebased to get rid of CI issues.
💬 sipa commented on pull request "net: transport abstraction":
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1684170559)
@Sjors I don't think so, or not easily. Giving the wrong "data_left" result just causes the #27981 heuristic to be wrong, but outside of some edge cases, that's unlikely to be noticeable in practice.
(https://github.com/bitcoin/bitcoin/pull/28165#issuecomment-1684170559)
@Sjors I don't think so, or not easily. Giving the wrong "data_left" result just causes the #27981 heuristic to be wrong, but outside of some edge cases, that's unlikely to be noticeable in practice.
🤔 fjahr reviewed a pull request: "test: tx orphan handling"
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1584785042)
Code review ACK 9eac5a0529f869075f0331e40d322c34fc8fc2af
...modulo removing the dead code. Otherwise mostly nit-ish comments, I can also open a follow-up myself if someone feels like merging this as is.
(https://github.com/bitcoin/bitcoin/pull/28199#pullrequestreview-1584785042)
Code review ACK 9eac5a0529f869075f0331e40d322c34fc8fc2af
...modulo removing the dead code. Otherwise mostly nit-ish comments, I can also open a follow-up myself if someone feels like merging this as is.
💬 fjahr commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298628089)
nit: I would argue that the (or the first) log should always be the first line of the test. Otherwise it may be confusing if an error happens in the code above it may look like the previous test failed.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298628089)
nit: I would argue that the (or the first) log should always be the first line of the test. Otherwise it may be confusing if an error happens in the code above it may look like the previous test failed.
💬 fjahr commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298585660)
This seems unused.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298585660)
This seems unused.
💬 fjahr commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298585453)
This function seems unused.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298585453)
This function seems unused.
💬 fjahr commented on pull request "test: tx orphan handling":
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298619617)
Could add another assertion here that `missing_parent_orphan` is indeed only requested once.
(https://github.com/bitcoin/bitcoin/pull/28199#discussion_r1298619617)
Could add another assertion here that `missing_parent_orphan` is indeed only requested once.