💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248159158)
> So I think if we now require that "The caller must ensure that upper_block has block data available." we should adjust the existing callers to do that.
Is there something you would like to see specifically, like adding asserts at the call site? I thought most of the call sites were just passing the chain tip, so it's hard to see how this could cause a problem unless the tip was pruned, in which case the current assert failure would be seem pretty appropriate
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248159158)
> So I think if we now require that "The caller must ensure that upper_block has block data available." we should adjust the existing callers to do that.
Is there something you would like to see specifically, like adding asserts at the call site? I thought most of the call sites were just passing the chain tip, so it's hard to see how this could cause a problem unless the tip was pruned, in which case the current assert failure would be seem pretty appropriate
📝 fanquake opened a pull request: "gui: drop macOS ForceActivation workaround"
(https://github.com/bitcoin-core/gui/pull/744)
Discussion in https://github.com/bitcoin/bitcoin/pull/16720 seems to point to this being no-longer needed after qt 5.6+.
We now require 5.11.x+.
(https://github.com/bitcoin-core/gui/pull/744)
Discussion in https://github.com/bitcoin/bitcoin/pull/16720 seems to point to this being no-longer needed after qt 5.6+.
We now require 5.11.x+.
💬 sipa commented on pull request "util: Allow FastRandomContext::randbytes for std::byte, Allow std::byte serialization":
(https://github.com/bitcoin/bitcoin/pull/28012#issuecomment-1615036465)
utACK fac6af16f4a254458b8cb3522317422b40362f8d
The two commits feel quite unrelated here, though, but both seem fine to me.
(https://github.com/bitcoin/bitcoin/pull/28012#issuecomment-1615036465)
utACK fac6af16f4a254458b8cb3522317422b40362f8d
The two commits feel quite unrelated here, though, but both seem fine to me.
👋 fanquake's pull request is ready for review: "ci: build Valgrind (3.21) from source"
(https://github.com/bitcoin/bitcoin/pull/27992)
(https://github.com/bitcoin/bitcoin/pull/27992)
💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248166855)
I don't think an assert failure is appropiate if having a pruned tip is a valid state that bitcoin core can be in, which seems to be the case, at least in the examples above. For example, it should be possible to call `getblockchaininfo` directly after loading a snapshot from disk, without triggering an assert.
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248166855)
I don't think an assert failure is appropiate if having a pruned tip is a valid state that bitcoin core can be in, which seems to be the case, at least in the examples above. For example, it should be possible to call `getblockchaininfo` directly after loading a snapshot from disk, without triggering an assert.
🚀 fanquake merged a pull request: "test: add python implementation of Elligator swift"
(https://github.com/bitcoin/bitcoin/pull/24005)
(https://github.com/bitcoin/bitcoin/pull/24005)
💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248168420)
Based on the `AssumeUTXO` line. It seems to be expected to not have block data available right after importing the chainstate. And that shouldn't crash the software if the user calls `pruneblockchain` or `getblockchaininfo`.
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248168420)
Based on the `AssumeUTXO` line. It seems to be expected to not have block data available right after importing the chainstate. And that shouldn't crash the software if the user calls `pruneblockchain` or `getblockchaininfo`.
💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248182230)
I see, so maybe current code is buggy and returns bad rpc results then? I guess simplest thing to do is just document GetFirstStoredBlock's odd behavior in this case and keep it unchanged, rather than returning nullptr which could segfault, or adding an assertion.
The new GetFirstStoredBlock function should be able to straightforwardly return false in this case, though.
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1248182230)
I see, so maybe current code is buggy and returns bad rpc results then? I guess simplest thing to do is just document GetFirstStoredBlock's odd behavior in this case and keep it unchanged, rather than returning nullptr which could segfault, or adding an assertion.
The new GetFirstStoredBlock function should be able to straightforwardly return false in this case, though.
💬 TheCharlatan commented on pull request "ci: re-enable gui tests for s390x":
(https://github.com/bitcoin/bitcoin/pull/28014#issuecomment-1615104483)
Post-merge ACK 9be4565c2db6d7a420d032d5c41843d473ed32d1
(https://github.com/bitcoin/bitcoin/pull/28014#issuecomment-1615104483)
Post-merge ACK 9be4565c2db6d7a420d032d5c41843d473ed32d1
💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248205239)
In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"
This doesn't actually apply the bumpfees to all of the outputs in `result. `All()` will return a copy of all of the outputs, not references to them, so this is just applying the bumpfees to a temporary.
Because of this, the tests added in this commit fail. They only do not fail because of the adjustment to the fee that is done in the following commit.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248205239)
In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"
This doesn't actually apply the bumpfees to all of the outputs in `result. `All()` will return a copy of all of the outputs, not references to them, so this is just applying the bumpfees to a temporary.
Because of this, the tests added in this commit fail. They only do not fail because of the adjustment to the fee that is done in the following commit.
💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248213082)
In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"
Compilation failure in this commit.
```
wallet/feebumper.cpp: In function ‘wallet::feebumper::Result wallet::CheckFeeRate(const CWallet&, const CMutableTransaction&, const CFeeRate&, int64_t, CAmount, std::vector<bilingual_str>&)’:
wallet/feebumper.cpp:89:61: error: ‘class interfaces::Chain’ has no member named ‘CalculateBumpFees’
89 | std::map<COutPoint, CAmount> bump_fees = wallet.chain(
...
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248213082)
In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"
Compilation failure in this commit.
```
wallet/feebumper.cpp: In function ‘wallet::feebumper::Result wallet::CheckFeeRate(const CWallet&, const CMutableTransaction&, const CFeeRate&, int64_t, CAmount, std::vector<bilingual_str>&)’:
wallet/feebumper.cpp:89:61: error: ‘class interfaces::Chain’ has no member named ‘CalculateBumpFees’
89 | std::map<COutPoint, CAmount> bump_fees = wallet.chain(
...
💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248207817)
In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"
nit: I prefer that comments about what a test does are attached to the test implementation itself rather than the caller.
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248207817)
In f2daebf8d1ac3829faa6443425cb97a763e9f780 "Bump unconfirmed parent txs to target feerate"
nit: I prefer that comments about what a test does are attached to the test implementation itself rather than the caller.
💬 achow101 commented on pull request "Bump unconfirmed ancestor transactions to target feerate":
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248219188)
This is incorrect, see https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248205239
(https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248219188)
This is incorrect, see https://github.com/bitcoin/bitcoin/pull/26152#discussion_r1248205239
🤔 mzumsande reviewed a pull request: "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1507793118)
Do I understand it correctly that using the new `forceinbound` permission instead of `noban` would be useful in the following two cases:
1.) If the user lowered `-maxconnections` to a value so low that the protection logic (that protects a fixed number between 20 and 28 inbound peers from eviction) could protect all inbound peers, but we still want to make sure to accept an inbound connection from a protected peer.
2) If we have a slightly higher `-maxconnections` but want to make sure a large
...
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1507793118)
Do I understand it correctly that using the new `forceinbound` permission instead of `noban` would be useful in the following two cases:
1.) If the user lowered `-maxconnections` to a value so low that the protection logic (that protects a fixed number between 20 and 28 inbound peers from eviction) could protect all inbound peers, but we still want to make sure to accept an inbound connection from a protected peer.
2) If we have a slightly higher `-maxconnections` but want to make sure a large
...
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1248242406)
No longer relevant
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1248242406)
No longer relevant
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1248242732)
Updated to use the mempool sequence number rather than `SteadyClock`
(https://github.com/bitcoin/bitcoin/pull/27675#discussion_r1248242732)
Updated to use the mempool sequence number rather than `SteadyClock`
💬 ajtowns commented on pull request "p2p: Drop m_recently_announced_invs bloom filter":
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1615138376)
Rebased and switched to using the mempool sequence instead of `SteadyClock`
(https://github.com/bitcoin/bitcoin/pull/27675#issuecomment-1615138376)
Rebased and switched to using the mempool sequence instead of `SteadyClock`
💬 sipa commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1615148105)
> I also spent some time whiteboarding whether package RBF could be feasible for non-v3 this way. I haven't found anything I'm happy with yet, but I think incorporating "cost to RBF conflicts" into the chunking algorithm could be a start.
I haven't thought hard about this, but package RBF sounds a whole lot more complex - and that's probably something that actually becomes easier to reason about post-clustermempool.
> One thing I want to preserve from the current approach is minimizing gro
...
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1615148105)
> I also spent some time whiteboarding whether package RBF could be feasible for non-v3 this way. I haven't found anything I'm happy with yet, but I think incorporating "cost to RBF conflicts" into the chunking algorithm could be a start.
I haven't thought hard about this, but package RBF sounds a whole lot more complex - and that's probably something that actually becomes easier to reason about post-clustermempool.
> One thing I want to preserve from the current approach is minimizing gro
...
💬 kevkevinpal commented on issue "init: torcontrol argument should be validated":
(https://github.com/bitcoin/bitcoin/issues/23589#issuecomment-1615162677)
I see it not throwing the error because it is returning true here if it has no `:`
https://github.com/bitcoin/bitcoin/blob/61d59fed74108f31eb4e9a2faa3f36422a37000e/src/util/strencodings.cpp#L117
I can open a PR to change this to false but it would affect these files that use `SplitHostPort` in `./bitcoin/src`
```
./src/test/fuzz/string.cpp:87
./src/test/netbase_tests.cpp:89
./src/util/strencodings.cpp:102
./src/util/strencodings.h:106
./src/bitcoin-cli.cpp:746
./src/net.cpp:547
.
...
(https://github.com/bitcoin/bitcoin/issues/23589#issuecomment-1615162677)
I see it not throwing the error because it is returning true here if it has no `:`
https://github.com/bitcoin/bitcoin/blob/61d59fed74108f31eb4e9a2faa3f36422a37000e/src/util/strencodings.cpp#L117
I can open a PR to change this to false but it would affect these files that use `SplitHostPort` in `./bitcoin/src`
```
./src/test/fuzz/string.cpp:87
./src/test/netbase_tests.cpp:89
./src/util/strencodings.cpp:102
./src/util/strencodings.h:106
./src/bitcoin-cli.cpp:746
./src/net.cpp:547
.
...
💬 santochibtc commented on pull request "Add feerate histogram to getmempoolinfo":
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1615177745)
I don't see the use of the histogram for fee estimation. You can have a lot of transactions with minimum fee that generate a large bar dominating the histogram. I see more useful a representation like mempool.space showing the list of transactions ordered by decreasing fee and grouped by blocks. That is a list of pending blocks with associated feerate ranges.
(https://github.com/bitcoin/bitcoin/pull/21422#issuecomment-1615177745)
I don't see the use of the histogram for fee estimation. You can have a lot of transactions with minimum fee that generate a large bar dominating the histogram. I see more useful a representation like mempool.space showing the list of transactions ordered by decreasing fee and grouped by blocks. That is a list of pending blocks with associated feerate ranges.