💬 maflcko commented on pull request "refactor: Replace sets of txiter with CTxMemPoolEntryRefs":
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954)
I wonder if it is possible to avoid copies of the entry. Currently one one copy is created when inserting into mapTx.
However, now that things are moving toward references, which are easy to copy accidentally. It may lead to bugs such as the one mentioned in the boost docs:
```
// The following, though similar to the previous code,
// does not work: iterator_to accepts a reference to
// the element in the container, not a copy.
int x=c.back();
c.erase(c.iterator_to(x)); // run-time fa
...
(https://github.com/bitcoin/bitcoin/pull/28886#issuecomment-1814794954)
I wonder if it is possible to avoid copies of the entry. Currently one one copy is created when inserting into mapTx.
However, now that things are moving toward references, which are easy to copy accidentally. It may lead to bugs such as the one mentioned in the boost docs:
```
// The following, though similar to the previous code,
// does not work: iterator_to accepts a reference to
// the element in the container, not a copy.
int x=c.back();
c.erase(c.iterator_to(x)); // run-time fa
...
📝 jonatack opened a pull request: "p2p: do not make automatic outbound connections to addnode peers"
(https://github.com/bitcoin/bitcoin/pull/28895)
to allocate our limited outbound slots correctly, and to ensure addnode
connections benefit from their intended protections.
Our addnode logic usually connects the addnode peers before the automatic
outbound logic does, but not always, as a connection race can occur. If an
addnode peer disconnects us and if it was the only one from its network, there
can be a race between reconnecting to it with the addnode thread, and it being
picked as automatic network-specific outbound peer. Or ou
...
(https://github.com/bitcoin/bitcoin/pull/28895)
to allocate our limited outbound slots correctly, and to ensure addnode
connections benefit from their intended protections.
Our addnode logic usually connects the addnode peers before the automatic
outbound logic does, but not always, as a connection race can occur. If an
addnode peer disconnects us and if it was the only one from its network, there
can be a race between reconnecting to it with the addnode thread, and it being
picked as automatic network-specific outbound peer. Or ou
...
💬 jonatack commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814836299)
Reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and extracted from an earlier version of https://github.com/bitcoin/bitcoin/pull/28248.
The first commit ought to be feasible to backport, if desired. The second test commit would require #28155.
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814836299)
Reported in https://github.com/bitcoin/bitcoin/pull/27213/files#r1291926369 and extracted from an earlier version of https://github.com/bitcoin/bitcoin/pull/28248.
The first commit ought to be feasible to backport, if desired. The second test commit would require #28155.
👍 theuni approved a pull request: "Remove version field from GetSerializeSize"
(https://github.com/bitcoin/bitcoin/pull/28878#pullrequestreview-1734964223)
ACK 83986f464c59a6517f790a960a72574e167f3f72.
I started working on a patch to remove `clientversion.h` where it's no longer necessary after these changes, but it's just more work than it's worth for this.
Can we just agree to do a (client)version.h include cleanup at some point when the dust has settled on all this? :)
(https://github.com/bitcoin/bitcoin/pull/28878#pullrequestreview-1734964223)
ACK 83986f464c59a6517f790a960a72574e167f3f72.
I started working on a patch to remove `clientversion.h` where it's no longer necessary after these changes, but it's just more work than it's worth for this.
Can we just agree to do a (client)version.h include cleanup at some point when the dust has settled on all this? :)
💬 theuni commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396049970)
Unrelated to this PR, but I think we could make this much more efficient by adding a serializer override that keeps a running tally of serialized size? That way size is calculated as a side-effect of the actual serialization and prepended to the stream, rather than running all the way through twice.
No idea if this path is hot enough to bother messing with that.
(https://github.com/bitcoin/bitcoin/pull/28878#discussion_r1396049970)
Unrelated to this PR, but I think we could make this much more efficient by adding a serializer override that keeps a running tally of serialized size? That way size is calculated as a side-effect of the actual serialization and prepended to the stream, rather than running all the way through twice.
No idea if this path is hot enough to bother messing with that.
💬 instagibbs commented on issue "wallet: Imports with pre-existing balance somtimes don't have any balance after a rescan":
(https://github.com/bitcoin/bitcoin/issues/19808#issuecomment-1814881584)
sorry, my issue is unrelated since a rescan fixed it:
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1466
I suspect my wallet birth date is newer than historical transactions my wallet has, causing it to miss things on reindex, which is a bit odd, but unrelated to this issue
(https://github.com/bitcoin/bitcoin/issues/19808#issuecomment-1814881584)
sorry, my issue is unrelated since a rescan fixed it:
https://github.com/bitcoin/bitcoin/blob/master/src/wallet/wallet.cpp#L1466
I suspect my wallet birth date is newer than historical transactions my wallet has, causing it to miss things on reindex, which is a bit odd, but unrelated to this issue
💬 miketwenty1 commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1814884903)
@petertodd
> Note that large coinbase transactions have caused a number of problems before,
including issues with ASIC compatibility and p2pool. We should not inflate
coinbase transaction size needlessly.
What responsibilities going forward do devs have with asic compatibility? Seems like a potential conflict of interest in some situations.
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1814884903)
@petertodd
> Note that large coinbase transactions have caused a number of problems before,
including issues with ASIC compatibility and p2pool. We should not inflate
coinbase transaction size needlessly.
What responsibilities going forward do devs have with asic compatibility? Seems like a potential conflict of interest in some situations.
💬 dergoegge commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814890913)
Thank you for isolating this change and making it more clear what you were going for.
From what I understand the "regression" is not that we might make automatic connections to addnode peers but rather that if that happens these connections might stick around as automatic connections for longer (due to the new eviction logic), while not being labeled as manual.
So in the end a user might end up with less outbound connections than expected for longer than is currently possible, with some of
...
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814890913)
Thank you for isolating this change and making it more clear what you were going for.
From what I understand the "regression" is not that we might make automatic connections to addnode peers but rather that if that happens these connections might stick around as automatic connections for longer (due to the new eviction logic), while not being labeled as manual.
So in the end a user might end up with less outbound connections than expected for longer than is currently possible, with some of
...
💬 jonatack commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814902180)
Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there. Since a couple of years I run a node with all three of our privacy networks and addnode entries for each, along with clearnet, and I didn't see my addnode peers being connected to as non-manual outbounds. This is new behavior for my node, it's frequent, it removes the intended privileges that addnode entries benefit from, and it impacts precisely the nodes running multiple networks i
...
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814902180)
Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there. Since a couple of years I run a node with all three of our privacy networks and addnode entries for each, along with clearnet, and I didn't see my addnode peers being connected to as non-manual outbounds. This is new behavior for my node, it's frequent, it removes the intended privileges that addnode entries benefit from, and it impacts precisely the nodes running multiple networks i
...
⚠️ lhajamine07 opened an issue: "White paper "
(https://github.com/bitcoin/bitcoin/issues/28896)
https://artifact.news/posts/p/3104766351295533632?s=8545508865735357353
(https://github.com/bitcoin/bitcoin/issues/28896)
https://artifact.news/posts/p/3104766351295533632?s=8545508865735357353
✅ fanquake closed an issue: "White paper "
(https://github.com/bitcoin/bitcoin/issues/28896)
(https://github.com/bitcoin/bitcoin/issues/28896)
💬 CryptAxe commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1814924908)
Concern over 33 bytes of data being added per block for blind merged mining seems disingenuous to me.
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1814924908)
Concern over 33 bytes of data being added per block for blind merged mining seems disingenuous to me.
👍 theuni approved a pull request: "refactor: Remove redundant checks in compat/assumptions.h"
(https://github.com/bitcoin/bitcoin/pull/28579#pullrequestreview-1735096323)
ACK fa1a38470697796a1a67397a815c8f8256f59224
(https://github.com/bitcoin/bitcoin/pull/28579#pullrequestreview-1735096323)
ACK fa1a38470697796a1a67397a815c8f8256f59224
💬 theuni commented on pull request "refactor: Remove redundant checks in compat/assumptions.h":
(https://github.com/bitcoin/bitcoin/pull/28579#discussion_r1396124973)
To be clear, issues (sorta kinda) like this are the ones I'm trying to guard against here: https://github.com/bitcoin/bitcoin/pull/27930 .
In that case, the definition of things changed in c++20/23. Ideally we'd be able to select exactly one behavior we're relying on (old or new) and fail to compile otherwise.
But yeah, I get that the state of things isn't perfect, so I don't care too much.
(https://github.com/bitcoin/bitcoin/pull/28579#discussion_r1396124973)
To be clear, issues (sorta kinda) like this are the ones I'm trying to guard against here: https://github.com/bitcoin/bitcoin/pull/27930 .
In that case, the definition of things changed in c++20/23. Ideally we'd be able to select exactly one behavior we're relying on (old or new) and fail to compile otherwise.
But yeah, I get that the state of things isn't perfect, so I don't care too much.
💬 RicYashiroLee commented on pull request "[WIP] BIP300 (Drivechains) consensus-level logic":
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1814963622)
> @petertodd
>
> > Note that large coinbase transactions have caused a number of problems before,
> > including issues with ASIC compatibility and p2pool. We should not inflate
> > coinbase transaction size needlessly.
>
> What responsibilities going forward do devs have with asic compatibility? Seems like a potential conflict of interest in some situations.
The reviewers of this and other PRs/BIPs also are in a position of high risk of conflict of interests. Every Dev wants to merge
...
(https://github.com/bitcoin/bitcoin/pull/28311#issuecomment-1814963622)
> @petertodd
>
> > Note that large coinbase transactions have caused a number of problems before,
> > including issues with ASIC compatibility and p2pool. We should not inflate
> > coinbase transaction size needlessly.
>
> What responsibilities going forward do devs have with asic compatibility? Seems like a potential conflict of interest in some situations.
The reviewers of this and other PRs/BIPs also are in a position of high risk of conflict of interests. Every Dev wants to merge
...
💬 dergoegge commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814977689)
> Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there.
Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.
I'm wondering why you only now noticed this, since the duration of these mislabeled connections prior to the new eviction logic (i assume) depended on regular outbound eviction behavior, which could also result in longer living connections.
...
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1814977689)
> Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there.
Sure but logically, the regression is that the duration of these connections increased not that they are being made in the first place.
I'm wondering why you only now noticed this, since the duration of these mislabeled connections prior to the new eviction logic (i assume) depended on regular outbound eviction behavior, which could also result in longer living connections.
...
⚠️ achow101 opened an issue: "Wallets should update key/descriptor birthdates when txs older than current birthdates are found"
(https://github.com/bitcoin/bitcoin/issues/28897)
### Please describe the feature you'd like to see added.
Key/descriptor birthdates should be updated to match the oldest transaction involving the key/descriptor when the timestamp of that transaction is older than the key/descriptor birthdate.
### Is your feature related to a problem, if so please describe it.
Users may fail to provide the correct birthdates for things that they imported, which results in the wallet storing incorrect birthdates. Since #27469, birthdates are used to determine
...
(https://github.com/bitcoin/bitcoin/issues/28897)
### Please describe the feature you'd like to see added.
Key/descriptor birthdates should be updated to match the oldest transaction involving the key/descriptor when the timestamp of that transaction is older than the key/descriptor birthdate.
### Is your feature related to a problem, if so please describe it.
Users may fail to provide the correct birthdates for things that they imported, which results in the wallet storing incorrect birthdates. Since #27469, birthdates are used to determine
...
💬 theStack commented on pull request "test: refactor: use built-in collection types for type hints (Python 3.9 / PEP 585)":
(https://github.com/bitcoin/bitcoin/pull/28725#issuecomment-1815000824)
@stickies-v: Thanks, good idea, I've added your suggested diff as a commit. As far as I can see, the only remaining uses of the `typing` module in the PR branch are now `Any`, `Optional`, `NoReturn` and `Union`, for which no replacements exist.
(https://github.com/bitcoin/bitcoin/pull/28725#issuecomment-1815000824)
@stickies-v: Thanks, good idea, I've added your suggested diff as a commit. As far as I can see, the only remaining uses of the `typing` module in the PR branch are now `Any`, `Optional`, `NoReturn` and `Union`, for which no replacements exist.
💬 jonatack commented on pull request "p2p: do not make automatic outbound connections to addnode peers":
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815011799)
> Thank you for isolating this change and making it more clear what you were going for.
The commit message in #28248 is quite similar to this one.
> From what I understand the "regression" is not that we might make automatic connections to addnode peers but rather that if that happens these connections might stick around as automatic connections for longer (due to the new eviction logic), while not being labeled as manual.
This stems from a change in this release where it was observed a
...
(https://github.com/bitcoin/bitcoin/pull/28895#issuecomment-1815011799)
> Thank you for isolating this change and making it more clear what you were going for.
The commit message in #28248 is quite similar to this one.
> From what I understand the "regression" is not that we might make automatic connections to addnode peers but rather that if that happens these connections might stick around as automatic connections for longer (due to the new eviction logic), while not being labeled as manual.
This stems from a change in this release where it was observed a
...
💬 romanz commented on pull request "rpc: keep `.cookie` file if it was not generated":
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1815031414)
Just wanted to ask whether there are any other outstanding issues? :)
(https://github.com/bitcoin/bitcoin/pull/28784#issuecomment-1815031414)
Just wanted to ask whether there are any other outstanding issues? :)