💬 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
...
💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1552459477)
Reviewers might be wondering what the benefits of the ResultTraits are. They are useful for potential future extensions of the move behaviour: https://github.com/bitcoin/bitcoin/pull/29700/commits/3951afc3b708326cea653951ef331d8f96a28682 .
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1552459477)
Reviewers might be wondering what the benefits of the ResultTraits are. They are useful for potential future extensions of the move behaviour: https://github.com/bitcoin/bitcoin/pull/29700/commits/3951afc3b708326cea653951ef331d8f96a28682 .
💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1552474458)
I have not quite grasped yet what the immediate benefit of these more generic decorators on the Messages are. Am I missing something from your draft PRs? I like shiny generics though, so to me a clear benefit of this approach would be to future-proof the result type. I was asking myself if this could be a bit more readable if instead of a forward-declared struct, the `MessagesTraits` were made a concept. After implementing it, I think it is indeed a bit easier to parse at the call site. It also
...
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1552474458)
I have not quite grasped yet what the immediate benefit of these more generic decorators on the Messages are. Am I missing something from your draft PRs? I like shiny generics though, so to me a clear benefit of this approach would be to future-proof the result type. I was asking myself if this could be a bit more readable if instead of a forward-declared struct, the `MessagesTraits` were made a concept. After implementing it, I think it is indeed a bit easier to parse at the call site. It also
...
💬 sipa commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552553259)
The `FeeFrac::size` field is an `int32_t`, which allows using the `FeeFrac{replacement_fees, replacement_vsize}` constructor below.
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552553259)
The `FeeFrac::size` field is an `int32_t`, which allows using the `FeeFrac{replacement_fees, replacement_vsize}` constructor below.
💬 sipa commented on pull request "feefrac: avoid explicitly computing diagram; compare based on chunks":
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552556360)
Good point, this isn't testing anymore at all. Gone.
(https://github.com/bitcoin/bitcoin/pull/29757#discussion_r1552556360)
Good point, this isn't testing anymore at all. Gone.
💬 theStack commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552562226)
removed
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552562226)
removed
💬 theStack commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552562328)
removed
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552562328)
removed
💬 theStack commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552562536)
done
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552562536)
done
💬 theStack commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552565066)
removed the duplicated `scriptSig_large` and added the assertion (small scriptSig, empty witness stack) as suggested
(https://github.com/bitcoin/bitcoin/pull/29777#discussion_r1552565066)
removed the duplicated `scriptSig_large` and added the assertion (small scriptSig, empty witness stack) as suggested
💬 theStack commented on pull request "test: refactor: introduce and use `calculate_input_weight` helper":
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2038425304)
Thanks for your reviews @kevkevinpal, @naiyoma, I force-pushed with all of your suggestions taken!
(https://github.com/bitcoin/bitcoin/pull/29777#issuecomment-2038425304)
Thanks for your reviews @kevkevinpal, @naiyoma, I force-pushed with all of your suggestions taken!
✅ achow101 closed an issue: "changed username"
(https://github.com/bitcoin/bitcoin/issues/29808)
(https://github.com/bitcoin/bitcoin/issues/29808)
:lock: achow101 locked an issue: "."
(https://github.com/bitcoin/bitcoin/issues/29808)
(https://github.com/bitcoin/bitcoin/issues/29808)
⚠️ tuttheking81 opened an issue: "."
(https://github.com/bitcoin/bitcoin/issues/29810)
.
_Originally posted by @tuttheking81 in https://github.com/bitcoin/bitcoin/issues/29808_
(https://github.com/bitcoin/bitcoin/issues/29810)
.
_Originally posted by @tuttheking81 in https://github.com/bitcoin/bitcoin/issues/29808_