💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812724875)
@mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812724875)
@mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
🚀 fanquake merged a pull request: "Use serialization parameters for CTransaction"
(https://github.com/bitcoin/bitcoin/pull/28438)
(https://github.com/bitcoin/bitcoin/pull/28438)
🚀 fanquake merged a pull request: "guix: update signapple (drop macho & altgraph)"
(https://github.com/bitcoin/bitcoin/pull/28859)
(https://github.com/bitcoin/bitcoin/pull/28859)
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812748534)
> I'd guess you could probably have waited for months to actually evict a mis-placed peer
In my testing it is frequent and systematic with our networks having a smaller number of peers. I believe these are the networks we're most interested in protecting with the new logic, and the ones for which users are likely to have addnode entries to ensure connection to the network.
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812748534)
> I'd guess you could probably have waited for months to actually evict a mis-placed peer
In my testing it is frequent and systematic with our networks having a smaller number of peers. I believe these are the networks we're most interested in protecting with the new logic, and the ones for which users are likely to have addnode entries to ensure connection to the network.
💬 mzumsande commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812752979)
> @mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
That's not my point. My point (see above) is that in earlier versions, the extra-outbound eviction (which would only happen in the stale-tip case then) was not a working or reliable mechanism to evict these peers, so there is no regression. Or did I miss something in my above explanation / do you have logs that pre-26 the stale-tip outbound eviction coul
...
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812752979)
> @mzumsande The improved logging allows distinguishing when this is due to the new protection logic, and with occasional exceptions, it is.
That's not my point. My point (see above) is that in earlier versions, the extra-outbound eviction (which would only happen in the stale-tip case then) was not a working or reliable mechanism to evict these peers, so there is no regression. Or did I miss something in my above explanation / do you have logs that pre-26 the stale-tip outbound eviction coul
...
💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1812753022)
@ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1812753022)
@ajtowns To avoid duplicate work, it would be good if you opened all follow-up pulls you wanted to do, or reply with a list here. Others can then take the remaining follow-ups :)
💬 maflcko commented on pull request "fuzz: Delete wallet_notifications":
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812754317)
> I believe it is not deterministic.
Can you add steps to reproduce?
(https://github.com/bitcoin/bitcoin/pull/28882#issuecomment-1812754317)
> I believe it is not deterministic.
Can you add steps to reproduce?
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812773694)
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.
Aside, I'm adding unit tests for each change right now, and these are finding additional issues.
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812773694)
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.
Aside, I'm adding unit tests for each change right now, and these are finding additional issues.
🤔 furszy reviewed a pull request: "wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs"
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1732331088)
Nice findings 👌🏼.
I haven't delved into investigating the reported BDB freeing issue within this context yet. But, can report that in the branch where I changed the underlying db migration mechanism ([link](https://github.com/furszy/bitcoin-core/tree/2023_wallet_safer_migration)), I haven't encountered the issue. I just pulled and ran all this PR new tests there and they all pass successfully in a loop.
One of the ideas of this work is to fix failures like 65b2da9 and others in a safer m
...
(https://github.com/bitcoin/bitcoin/pull/28868#pullrequestreview-1732331088)
Nice findings 👌🏼.
I haven't delved into investigating the reported BDB freeing issue within this context yet. But, can report that in the branch where I changed the underlying db migration mechanism ([link](https://github.com/furszy/bitcoin-core/tree/2023_wallet_safer_migration)), I haven't encountered the issue. I just pulled and ran all this PR new tests there and they all pass successfully in a loop.
One of the ideas of this work is to fix failures like 65b2da9 and others in a safer m
...
👍 hebasto approved a pull request: "doc: remove mention of missing bdb being a configure error"
(https://github.com/bitcoin/bitcoin/pull/28881#pullrequestreview-1732347366)
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc.
(https://github.com/bitcoin/bitcoin/pull/28881#pullrequestreview-1732347366)
ACK 30bd4b1e4aee00edbe77da7c20bf80e28f0561cc.
📝 muxator opened a pull request: "contrib: use a raw string for a regular expression literal that contains backslashes in signet/miner"
(https://github.com/bitcoin/bitcoin/pull/28883)
Running `contrib/signet/miner` under python >= 3.12 causes a `SyntaxWarning`. The problem was already present in previous versions, but it only triggered a `DeprecationWarning`, which was not shown by default.
The change is useful for future-proofing the code base, since future python versions will start to exit with a runtime exception (see the reference given later).
Command to see the warning at runtime under python3.11 (`DeprecationWarning`, needs "-Walways"):
```
$ python3.11 -Walwa
...
(https://github.com/bitcoin/bitcoin/pull/28883)
Running `contrib/signet/miner` under python >= 3.12 causes a `SyntaxWarning`. The problem was already present in previous versions, but it only triggered a `DeprecationWarning`, which was not shown by default.
The change is useful for future-proofing the code base, since future python versions will start to exit with a runtime exception (see the reference given later).
Command to see the warning at runtime under python3.11 (`DeprecationWarning`, needs "-Walways"):
```
$ python3.11 -Walwa
...
💬 jonatack commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1394406381)
Yes, I'm going to punt on this commit for now. Seeing dozens to hundreds of connection attempts per day from, say, 1-2 inbound I2P peers that are already connected. However, I'm not sure these peers are malicious, and the behavior might be due to other issues in our already connected detection or outbound logic addressed in this PR.
(https://github.com/bitcoin/bitcoin/pull/28248#discussion_r1394406381)
Yes, I'm going to punt on this commit for now. Seeing dozens to hundreds of connection attempts per day from, say, 1-2 inbound I2P peers that are already connected. However, I'm not sure these peers are malicious, and the behavior might be due to other issues in our already connected detection or outbound logic addressed in this PR.
💬 maflcko commented on pull request "contrib: use a raw string for a regular expression literal that contains backslashes in signet/miner":
(https://github.com/bitcoin/bitcoin/pull/28883#issuecomment-1812805851)
lgtm ACK defdf67765a3d757f4d3840602eef7ccdac9bb49
(https://github.com/bitcoin/bitcoin/pull/28883#issuecomment-1812805851)
lgtm ACK defdf67765a3d757f4d3840602eef7ccdac9bb49
💬 mzumsande commented on pull request "p2p: Increase inbound capacity for block-relay only connections":
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-1812811783)
> I'd argue that this is a significant change.
Hmm yes, in that comment I was thinking about the theoretical limit of all inbound slots being filled with tx-relaying peers. You are right, in practice that amount will be lower because some block-relay-only connections are made to us. In that sense, the introduction of block-relay-only peers likely came with a reduction in bandwidth for popular nodes that were already at capacity before.
One way to accommodate this would be to reduce the per
...
(https://github.com/bitcoin/bitcoin/pull/28463#issuecomment-1812811783)
> I'd argue that this is a significant change.
Hmm yes, in that comment I was thinking about the theoretical limit of all inbound slots being filled with tx-relaying peers. You are right, in practice that amount will be lower because some block-relay-only connections are made to us. In that sense, the introduction of block-relay-only peers likely came with a reduction in bandwidth for popular nodes that were already at capacity before.
One way to accommodate this would be to reduce the per
...
💬 TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1812821242)
Rebased 40e151697d3d1ed594364498d9e6220219d9b545 -> de6d79847537cd7d0dfc3e2dd7a57b39b4281b5a ([kernelInternalLib_5](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_5) -> [kernelInternalLib_6](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_5..kernelInternalLib_6))
* Fixed merge conflict.
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-1812821242)
Rebased 40e151697d3d1ed594364498d9e6220219d9b545 -> de6d79847537cd7d0dfc3e2dd7a57b39b4281b5a ([kernelInternalLib_5](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_5) -> [kernelInternalLib_6](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_6), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_5..kernelInternalLib_6))
* Fixed merge conflict.
💬 MatthewLM commented on pull request "Use LE hex-encoded representations in script ASM for pushed values <= 4 bytes":
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812829602)
Could we use `UNKNOWN_n` where n denotes the op code hex/decimal such as `UNKOWN_FE` for `0xfe`?
(https://github.com/bitcoin/bitcoin/pull/28824#issuecomment-1812829602)
Could we use `UNKNOWN_n` where n denotes the op code hex/decimal such as `UNKOWN_FE` for `0xfe`?
💬 mzumsande commented on pull request "p2p: peer connection bug fixes":
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812859356)
> Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there.
Oh, I thought you were talking about a race at startup - since the extra outbound should only kick in after 5 minutes, I think that should be enough to make connections to the addnode peers.
I think what might be happening later is that an addnoded peer disconnects us, and if it was the only one from its network then there could be a race between picking another network-rela
...
(https://github.com/bitcoin/bitcoin/pull/28248#issuecomment-1812859356)
> Empirically, the regression is that we're actively adding addnode peers as outbound ones, and keeping them there.
Oh, I thought you were talking about a race at startup - since the extra outbound should only kick in after 5 minutes, I think that should be enough to make connections to the addnode peers.
I think what might be happening later is that an addnoded peer disconnects us, and if it was the only one from its network then there could be a race between picking another network-rela
...
💬 theuni commented on pull request "Remove version field from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/28878#issuecomment-1812867807)
Concept ACK.
Is this next after #28438? Or something else first?
(https://github.com/bitcoin/bitcoin/pull/28878#issuecomment-1812867807)
Concept ACK.
Is this next after #28438? Or something else first?
💬 Riahiamirreza commented on pull request "rpc: show P2(W)SH redeemScript in getrawtransaction #27637":
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1812892050)
I'm working on adding tests, but I'm not sure where is the correct place for putting tests. Should I put tests in `RawTransactionsTest.getrawtransaction_verbositry_tests` in the `tests/functional/rpc_rawtransaction.py`? I need to have a transaction of type P2SH in the chain and check whether the result has the correct `redeemScript` field? Is this approach correct?
(https://github.com/bitcoin/bitcoin/pull/27638#issuecomment-1812892050)
I'm working on adding tests, but I'm not sure where is the correct place for putting tests. Should I put tests in `RawTransactionsTest.getrawtransaction_verbositry_tests` in the `tests/functional/rpc_rawtransaction.py`? I need to have a transaction of type P2SH in the chain and check whether the result has the correct `redeemScript` field? Is this approach correct?
👍 fanquake approved a pull request: "bench: Update nanobench to 4.3.11"
(https://github.com/bitcoin/bitcoin/pull/28877#pullrequestreview-1732483272)
ACK fe434a469534766f18d7560d968deed37193835f - have not tested.
(https://github.com/bitcoin/bitcoin/pull/28877#pullrequestreview-1732483272)
ACK fe434a469534766f18d7560d968deed37193835f - have not tested.