💬 ryanofsky commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2059591077)
This seems like an improvement, but I think the original suggestion of just adding a `std::error& error` output parameter to the CTxMemPool constructor (or a `util::Result<void>& result` output parameter) would provide a less rigid API than making the CTxMemPool constructor private and requiring every instance to be allocated on the heap and owned by a unique_ptr. This would also allow keeping the fuzz test behavior unchanged. The new fuzz test behavior is probably better, but maybe fuzz testing
...
(https://github.com/bitcoin/bitcoin/pull/28830#issuecomment-2059591077)
This seems like an improvement, but I think the original suggestion of just adding a `std::error& error` output parameter to the CTxMemPool constructor (or a `util::Result<void>& result` output parameter) would provide a less rigid API than making the CTxMemPool constructor private and requiring every instance to be allocated on the heap and owned by a unique_ptr. This would also allow keeping the fuzz test behavior unchanged. The new fuzz test behavior is probably better, but maybe fuzz testing
...
💬 laanwj commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1567731932)
Would it be possible to change these to values instead of defined/undefined? It seems to me that changing these to `ifdef` makes it less safe because you lose the protection added by the warning in the first place. (say, when you forget to include bitcoin-config)
(https://github.com/bitcoin/bitcoin/pull/29876#discussion_r1567731932)
Would it be possible to change these to values instead of defined/undefined? It seems to me that changing these to `ifdef` makes it less safe because you lose the protection added by the warning in the first place. (say, when you forget to include bitcoin-config)
👍 alfonsoromanz approved a pull request: "test: fix intermittent failure in p2p_compactblocks_hb.py"
(https://github.com/bitcoin/bitcoin/pull/29893#pullrequestreview-2004262314)
Tested ACK 1ae5b208d339fa984d9caf4fab89b0b2ba9cc197
(https://github.com/bitcoin/bitcoin/pull/29893#pullrequestreview-2004262314)
Tested ACK 1ae5b208d339fa984d9caf4fab89b0b2ba9cc197
⚠️ maflcko opened an issue: "Intermittent issue in p2p_unrequested_blocks.py", line 98, in run_test test_node.send_and_ping(msg_block(blocks_h2[0])) assert self.is_connected AssertionError / AcceptBlock FAILED (time-too-old, block's timestamp is too early) "
(https://github.com/bitcoin/bitcoin/issues/29897)
https://github.com/bitcoin/bitcoin/actions/runs/8689751788/job/23828088498?pr=29877#step:27:584
```
node0 2024-04-15T13:27:36.008782Z [D:\a\bitcoin\bitcoin\src\net_processing.cpp:4743] [ProcessMessage] [net] received block 20076ff9e9d9bace59d7621747b34dfb4af9b2f75ff499017397854f9fefb930 peer=0
node0 2024-04-15T13:27:36.008822Z [D:\a\bitcoin\bitcoin\src\validation.cpp:4145] [AcceptBlockHeader] [validation] AcceptBlockHeader: Consensus::ContextualCheckBlockHeader: 20076ff9e9d9bace59d7621747
...
(https://github.com/bitcoin/bitcoin/issues/29897)
https://github.com/bitcoin/bitcoin/actions/runs/8689751788/job/23828088498?pr=29877#step:27:584
```
node0 2024-04-15T13:27:36.008782Z [D:\a\bitcoin\bitcoin\src\net_processing.cpp:4743] [ProcessMessage] [net] received block 20076ff9e9d9bace59d7621747b34dfb4af9b2f75ff499017397854f9fefb930 peer=0
node0 2024-04-15T13:27:36.008822Z [D:\a\bitcoin\bitcoin\src\validation.cpp:4145] [AcceptBlockHeader] [validation] AcceptBlockHeader: Consensus::ContextualCheckBlockHeader: 20076ff9e9d9bace59d7621747
...
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567724841)
would be good to explicitly test that the parent-giver isn't punished in this scenario, and test if parent is consensus-bad it results in something expected.
```
diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
index 603cbf08a9..1c887289dc 100755
--- a/test/functional/p2p_opportunistic_1p1c.py
+++ b/test/functional/p2p_opportunistic_1p1c.py
@@ -164,78 +164,129 @@ class PackageRelayTest(BitcoinTestFramework):
# itself and with a
...
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567724841)
would be good to explicitly test that the parent-giver isn't punished in this scenario, and test if parent is consensus-bad it results in something expected.
```
diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
index 603cbf08a9..1c887289dc 100755
--- a/test/functional/p2p_opportunistic_1p1c.py
+++ b/test/functional/p2p_opportunistic_1p1c.py
@@ -164,78 +164,129 @@ class PackageRelayTest(BitcoinTestFramework):
# itself and with a
...
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567718964)
```Suggestion
# 3. Sender relays the parent. Parent+Child are evaluated as a package and rejected.
```
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567718964)
```Suggestion
# 3. Sender relays the parent. Parent+Child are evaluated as a package and rejected.
```
💬 pinheadmz commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059641989)
Rebasing on master since one commit was merged separately in #29649
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059641989)
Rebasing on master since one commit was merged separately in #29649
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2059650726)
Thanks for the update! I noticed you didn't include Ocean. While they aren't of significant hash power, they do in fact do full-rbf for some of their hash power as they give miners an option of picking Knots (full-rbf) or Core (defaults) templates.
Also, Wasabi is now relying on full-rbf in production: https://github.com/zkSNACKs/WalletWasabi/pull/12677
tl;dr: their coordinator assumes that if a double spend is detected of lower fee-rate, the coinjoin will most likely win anyway regardless of
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-2059650726)
Thanks for the update! I noticed you didn't include Ocean. While they aren't of significant hash power, they do in fact do full-rbf for some of their hash power as they give miners an option of picking Knots (full-rbf) or Core (defaults) templates.
Also, Wasabi is now relying on full-rbf in production: https://github.com/zkSNACKs/WalletWasabi/pull/12677
tl;dr: their coordinator assumes that if a double spend is detected of lower fee-rate, the coinjoin will most likely win anyway regardless of
...
✅ maflcko closed an issue: "Memory leak via API listtransactions"
(https://github.com/bitcoin/bitcoin/issues/25229)
(https://github.com/bitcoin/bitcoin/issues/25229)
💬 maflcko commented on issue "Memory leak via API listtransactions":
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059651519)
I don't think there is anything that can be done here. The final JSON RPC reply fully written to a single byte buffer must be stored in memory contiguously. If such an amount of memory is not available, it needs to be allocated.
In theory it could be returned, for example by `malloc_trim`, but what would be the point, if on the next RPC call it will need to be allocated again?
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059651519)
I don't think there is anything that can be done here. The final JSON RPC reply fully written to a single byte buffer must be stored in memory contiguously. If such an amount of memory is not available, it needs to be allocated.
In theory it could be returned, for example by `malloc_trim`, but what would be the point, if on the next RPC call it will need to be allocated again?
💬 maflcko commented on issue "Memory leak via API listtransactions":
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059656053)
If you are looking for a workaround, you can either provide more memory (or swap), or you could try to call the RPC with smaller `count` values.
(https://github.com/bitcoin/bitcoin/issues/25229#issuecomment-2059656053)
If you are looking for a workaround, you can either provide more memory (or swap), or you could try to call the RPC with smaller `count` values.
💬 achow101 commented on pull request "doc: Add example of mixing private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/28373#issuecomment-2059662567)
ACK 24b67fa9f602cdeac0e9736256f77d048f616c48
(https://github.com/bitcoin/bitcoin/pull/28373#issuecomment-2059662567)
ACK 24b67fa9f602cdeac0e9736256f77d048f616c48
💬 achow101 commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1566472098)
In 000000000c5fad4aca360a6e914c7779167838db "Add bech32 error short & multiple separators cases"
This is incorrect. `1` is an allowed character in the human readable part, hence a multiple separators check doesn't make sense. The real separator is simply the last `1` that appears. See https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1566472098)
In 000000000c5fad4aca360a6e914c7779167838db "Add bech32 error short & multiple separators cases"
This is incorrect. `1` is an allowed character in the human readable part, hence a multiple separators check doesn't make sense. The real separator is simply the last `1` that appears. See https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#bech32
💬 instagibbs commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567763258)
suggestion:
call this `med_fee_child` and use the mempoolminfee directly since that's a more meaningful value than `2*FEERATE_1SAT_VB` which is below the floating minfee
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567763258)
suggestion:
call this `med_fee_child` and use the mempoolminfee directly since that's a more meaningful value than `2*FEERATE_1SAT_VB` which is below the floating minfee
💬 laanwj commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567771033)
`header.type` is compared against `BTREE_LEAF` and `BTREE_INTERNAL`, which are the same cases as handled in the `switch()` statement below, which also errors in the default page (though with a slightly different error), so this check is redundant?
(https://github.com/bitcoin/bitcoin/pull/26606#discussion_r1567771033)
`header.type` is compared against `BTREE_LEAF` and `BTREE_INTERNAL`, which are the same cases as handled in the `switch()` statement below, which also errors in the default page (though with a slightly different error), so this check is redundant?
💬 achow101 commented on pull request "Enhanced error messages for invalid network prefix during address parsing.":
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610)
In 000000000fbad5312fe6975959b0ab9ca5daeb16 "Add Base58 Address Prefix Decoder"
It seems a bit odd to me to do all of this work to compute these prefixes just for error messaging. ISTM it would be a lot simpler to just hard code them into chainparams anyways. It's not as if the prefixes will ever change, and new ones are unlikely to be added.
Merging this code seems like an additional maintenance burden for very little gain.
Also, if you insist on keeping this code, it should be placed
...
(https://github.com/bitcoin/bitcoin/pull/27260#discussion_r1567767610)
In 000000000fbad5312fe6975959b0ab9ca5daeb16 "Add Base58 Address Prefix Decoder"
It seems a bit odd to me to do all of this work to compute these prefixes just for error messaging. ISTM it would be a lot simpler to just hard code them into chainparams anyways. It's not as if the prefixes will ever change, and new ones are unlikely to be added.
Merging this code seems like an additional maintenance burden for very little gain.
Also, if you insist on keeping this code, it should be placed
...
💬 pinheadmz commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059679632)
Thanks so much for your review @laanwj addressed your comments
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2059679632)
Thanks so much for your review @laanwj addressed your comments
💬 pinheadmz commented on pull request "system: use %LOCALAPPDATA% as default datadir on windows":
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2059694500)
> Could rebase for fresh CI, if still relevant?
rebased on master
(https://github.com/bitcoin/bitcoin/pull/27064#issuecomment-2059694500)
> Could rebase for fresh CI, if still relevant?
rebased on master
💬 ryanofsky commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2059695927)
> Though maybe if the timing is really unlucky you'd get lots of Commit() calls?
That's a good point. If Commit() is slow and it has to be called more than once, it could make index sync slower, even if it blocks validation code less.
There's a choice about what to do when the Commit() call at the end of the sync is slow and a new block is connected while it is executing.
1. Currently, the new block may get ignored and the index could be corrupted.
2. Before 0faafb57f8298547949cbc0044e
...
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2059695927)
> Though maybe if the timing is really unlucky you'd get lots of Commit() calls?
That's a good point. If Commit() is slow and it has to be called more than once, it could make index sync slower, even if it blocks validation code less.
There's a choice about what to do when the Commit() call at the end of the sync is slow and a new block is connected while it is executing.
1. Currently, the new block may get ignored and the index could be corrupted.
2. Before 0faafb57f8298547949cbc0044e
...
💬 pinheadmz commented on pull request "blockstorage: do not flush block to disk if it is already there":
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567788078)
I'm not sure I understand. The previous code expected a crash so the test would know exactly when to check for the expected log message. In the new code bitcoind just runs fine so we have to poll the log for the success message.
(https://github.com/bitcoin/bitcoin/pull/27039#discussion_r1567788078)
I'm not sure I understand. The previous code expected a crash so the test would know exactly when to check for the expected log message. In the new code bitcoind just runs fine so we have to poll the log for the success message.