⚠️ maflcko opened an issue: "intermittent issue in wallet_backwards_compatibility.py: line 245, in run_test assert txs[3]["abandoned"] AssertionError"
(https://github.com/bitcoin/bitcoin/issues/29806)
https://cirrus-ci.com/task/5524545770094592?logs=ci#L3290
(https://github.com/bitcoin/bitcoin/issues/29806)
https://cirrus-ci.com/task/5524545770094592?logs=ci#L3290
💬 Eunovo commented on pull request "Wallet: Add `max_tx_weight` to transaction funding options (take 2)":
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2037477967)
Tested ACK https://github.com/bitcoin/bitcoin/pull/29523/commits/3a64b3a339fe55c2642369b00a382231871c283
(https://github.com/bitcoin/bitcoin/pull/29523#issuecomment-2037477967)
Tested ACK https://github.com/bitcoin/bitcoin/pull/29523/commits/3a64b3a339fe55c2642369b00a382231871c283
💬 pablomartin4btc commented on pull request "Don't permit port in proxy IP option":
(https://github.com/bitcoin-core/gui/pull/813#issuecomment-2037524282)
> I'm not convinced that a large regex is "simpler" than checking if a ":" is in the string?
The large one was for the port number, the one I referred as "simpler" was for the IP address: `QRegularExpression ipRegex("^([0-9]{1,3}\\.){3}[0-9]{1,3}$");`. What I meant is that we know in advance that the ":" would be invalid so we could restrict the user to even enter it. As current we could see this (not only ":"):
> I'm not convinced that a large regex is "simpler" than checking if a ":" is in the string?
The large one was for the port number, the one I referred as "simpler" was for the IP address: `QRegularExpression ipRegex("^([0-9]{1,3}\\.){3}[0-9]{1,3}$");`. What I meant is that we know in advance that the ":" would be invalid so we could restrict the user to even enter it. As current we could see this (not only ":"):
![image](https://github.com/bitcoin-core/gui/assets/110166421/139224e2-aa0d-44
...
💬 fanquake commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2037552298)
One thing to investigate is if `-ggdb(n)` is useful here at all.
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2037552298)
One thing to investigate is if `-ggdb(n)` is useful here at all.
💬 glozow commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552024237)
nit: why change to int32?
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552024237)
nit: why change to int32?
💬 glozow commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552029703)
nit: Could probably just delete the full test case? This isn't a `build_diagram_test` any more, and comparison of oversized chunks is already tested above.
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552029703)
nit: Could probably just delete the full test case? This isn't a `build_diagram_test` any more, and comparison of oversized chunks is already tested above.
🤔 glozow reviewed a pull request: "feefrac: avoid explicitly computing diagram; compare based on chunks"
(https://github.com/bitcoin/bitcoin/pull/29757#pullrequestreview-1980465571)
code review ACK 43e2f6d160b75e88b39777428c2c1892b962f394
(https://github.com/bitcoin/bitcoin/pull/29757#pullrequestreview-1980465571)
code review ACK 43e2f6d160b75e88b39777428c2c1892b962f394
💬 krisisnotok commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2037674193)
? How, like how? My bitcoins are gone?
On Thu, 4 Apr 2024 at 9:59 PM, Gloria Zhao ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> code review ACK 43e2f6d
> <https://github.com/bitcoin/bitcoin/commit/43e2f6d160b75e88b39777428c2c1892b962f394>
> ------------------------------
>
> In src/test/fuzz/rbf.cpp
> <https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552024237>:
>
> > CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider);
> - int
...
(https://github.com/bitcoin/bitcoin/pull/29757#issuecomment-2037674193)
? How, like how? My bitcoins are gone?
On Thu, 4 Apr 2024 at 9:59 PM, Gloria Zhao ***@***.***> wrote:
> ***@***.**** commented on this pull request.
>
> code review ACK 43e2f6d
> <https://github.com/bitcoin/bitcoin/commit/43e2f6d160b75e88b39777428c2c1892b962f394>
> ------------------------------
>
> In src/test/fuzz/rbf.cpp
> <https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552024237>:
>
> > CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider);
> - int
...
💬 davidgumberg commented on pull request "p2p: gives seednode priority over dnsseed if both are provided":
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2037768188)
reACK https://github.com/bitcoin/bitcoin/commit/2842e51a246b162a586941184b7694f187d7aee7
I think the 30 second dns seed fallback condition here is more likely than I assumed because of the low quality of P2P gossip addresses mentioned by mzumsande above. I experienced it on my first test of this branch on signet and decided to investigate:
In my testing, 6 out of 12 attempts to bootstrap a signet node with a `-seednode` fell back to DNS seeds, and 7 out of 12 attempts to bootstrap a mainne
...
(https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-2037768188)
reACK https://github.com/bitcoin/bitcoin/commit/2842e51a246b162a586941184b7694f187d7aee7
I think the 30 second dns seed fallback condition here is more likely than I assumed because of the low quality of P2P gossip addresses mentioned by mzumsande above. I experienced it on my first test of this branch on signet and decided to investigate:
In my testing, 6 out of 12 attempts to bootstrap a signet node with a `-seednode` fell back to DNS seeds, and 7 out of 12 attempts to bootstrap a mainne
...
🤔 murchandamus reviewed a pull request: "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure"
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1980784495)
ACK 14c86ba721e1a208c88ada133ba9e90e24724ea4 with nits.
(https://github.com/bitcoin/bitcoin/pull/29735#pullrequestreview-1980784495)
ACK 14c86ba721e1a208c88ada133ba9e90e24724ea4 with nits.
💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552208469)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (271599af6d84c871154dc16c7abcadbe5ac08163):
If these node parameters generally are needed for `fill_mempool` to work, would it perhaps make sense to move them to the `fill_mempool` function?
Alternatively, the comment should perhaps be "Lower mempool limit to make it easier to `fill_mempool`"
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552208469)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (271599af6d84c871154dc16c7abcadbe5ac08163):
If these node parameters generally are needed for `fill_mempool` to work, would it perhaps make sense to move them to the `fill_mempool` function?
Alternatively, the comment should perhaps be "Lower mempool limit to make it easier to `fill_mempool`"
💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552198747)
In "Move fill_mempool to util function" (cc770e4b36e6f04cbd588ad3e7a19f122c21d38d):
Nit: If a commit is described as a move, I would expect no changes in the code beyond what’s necessary to update the callsites. While these changes are minor and obviously do not entail a functional change, I would have expected the updated commentary and refactor to be a separate commit—I’m wondering whether my understanding of the modus operandi is correct.
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552198747)
In "Move fill_mempool to util function" (cc770e4b36e6f04cbd588ad3e7a19f122c21d38d):
Nit: If a commit is described as a move, I would expect no changes in the code beyond what’s necessary to update the callsites. While these changes are minor and obviously do not entail a functional change, I would have expected the updated commentary and refactor to be a separate commit—I’m wondering whether my understanding of the modus operandi is correct.
💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552232875)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):
Pet-peeve nit: I suspect that the issues here is rather that the mempool min _feerate_ is not met?
```suggestion
assert "mempool min feerate not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]
```
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552232875)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):
Pet-peeve nit: I suspect that the issues here is rather that the mempool min _feerate_ is not met?
```suggestion
assert "mempool min feerate not met" in pkg_result["tx-results"][parent["wtxid"]]["error"]
```
💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552235727)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):
Did you perhaps mean:
```suggestion
# Reset maxmempool and datacarriersize and empty mempool to reset dynamic mempool minimum feerate
```
?
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552235727)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):
Did you perhaps mean:
```suggestion
# Reset maxmempool and datacarriersize and empty mempool to reset dynamic mempool minimum feerate
```
?
💬 murchandamus commented on pull request "AcceptMultipleTransactions: Fix workspace not being set as client_maxfeerate failure":
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552244211)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):
At this point, it might be better to split the `maxfeerate` and the `maxburnamount` tests.
(https://github.com/bitcoin/bitcoin/pull/29735#discussion_r1552244211)
In "AcceptMultipleTransactions: Fix workspace not being set as client_max…
…feerate failure" (https://github.com/bitcoin/bitcoin/commit/271599af6d84c871154dc16c7abcadbe5ac08163):
At this point, it might be better to split the `maxfeerate` and the `maxburnamount` tests.
💬 sdaftuar commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#issuecomment-2037980109)
Concept and code review ACK. I haven't done any testing (nor did I carefully review the tests), just read through the code and thought about the overall logical flow, which makes sense to me.
(https://github.com/bitcoin/bitcoin/pull/29575#issuecomment-2037980109)
Concept and code review ACK. I haven't done any testing (nor did I carefully review the tests), just read through the code and thought about the overall logical flow, which makes sense to me.
⚠️ tuttheking81 opened an issue: "changed username"
(https://github.com/bitcoin/bitcoin/issues/29808)
changed username
_Originally posted by @fabianonetto in https://github.com/octocat/Spoon-Knife/pull/32783_
(https://github.com/bitcoin/bitcoin/issues/29808)
changed username
_Originally posted by @fabianonetto in https://github.com/octocat/Spoon-Knife/pull/32783_
📝 naiyoma opened a pull request: "net: update comment for service bit support info for seed.bitcoin.sipa.be"
(https://github.com/bitcoin/bitcoin/pull/29809)
This PR updates the comment regarding the supported service bits for `seed.bitcoin.sipa.be` based on this reference: https://github.com/sipa/bitcoin-seeder/blob/ff482e465ff84ea6fa276d858ccb7ef32e3355d3/main.cpp#L208
(https://github.com/bitcoin/bitcoin/pull/29809)
This PR updates the comment regarding the supported service bits for `seed.bitcoin.sipa.be` based on this reference: https://github.com/sipa/bitcoin-seeder/blob/ff482e465ff84ea6fa276d858ccb7ef32e3355d3/main.cpp#L208
🤔 furszy reviewed a pull request: "Don't permit port in proxy IP option"
(https://github.com/bitcoin-core/gui/pull/813#pullrequestreview-1981135576)
utACK 10c5275ba45
(https://github.com/bitcoin-core/gui/pull/813#pullrequestreview-1981135576)
utACK 10c5275ba45
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2038255584)
Rebased and ran some faster benchmarks, 2 each but only until 400k. Will do some more going to 800k.
| | prune | dbcache | mean time (s) | speedup |
|-----------:|----------:|------------:|--------:|--------------:|
| master | 550 | 16384 | 2,018 | - |
| branch | 550 | 16384 | 1,667 | 17.4% |
| master | 550 | 450 | 2,069 | - |
| branch | 550 | 450 | 1,822 | 11.9% |
| master | 0 | 16384 | 1,447 | - |
| branch | 0 | 16384 | 1,396 | 3.5% |
| master | 0 | 450 | 1,546 | - |
| branch | 0
...
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2038255584)
Rebased and ran some faster benchmarks, 2 each but only until 400k. Will do some more going to 800k.
| | prune | dbcache | mean time (s) | speedup |
|-----------:|----------:|------------:|--------:|--------------:|
| master | 550 | 16384 | 2,018 | - |
| branch | 550 | 16384 | 1,667 | 17.4% |
| master | 550 | 450 | 2,069 | - |
| branch | 550 | 450 | 1,822 | 11.9% |
| master | 0 | 16384 | 1,447 | - |
| branch | 0 | 16384 | 1,396 | 3.5% |
| master | 0 | 450 | 1,546 | - |
| branch | 0
...