👍 TheCharlatan approved a pull request: "validation: improve m_best_header tracking"
(https://github.com/bitcoin/bitcoin/pull/30666#pullrequestreview-2244388019)
ACK c9bf06a531617fdd70b64e23572931af2b969828
(https://github.com/bitcoin/bitcoin/pull/30666#pullrequestreview-2244388019)
ACK c9bf06a531617fdd70b64e23572931af2b969828
💬 TheCharlatan commented on pull request "validation: improve m_best_header tracking":
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1721048165)
It might be a good idea to expand this a bit:
```diff
@@ -45 +45 @@ class InvalidateTest(BitcoinTestFramework):
- tip = int(self.nodes[0].getbestblockhash(), 16)
+ tip = self.nodes[0].getbestblockhash()
@@ -47 +47 @@ class InvalidateTest(BitcoinTestFramework):
- block = create_block(tip, create_coinbase(self.nodes[0].getblockcount()), block_time, version=4)
+ block = create_block(int(tip, 16), create_coinbase(self.nodes[0].getblockcount()), block_time, version=
...
(https://github.com/bitcoin/bitcoin/pull/30666#discussion_r1721048165)
It might be a good idea to expand this a bit:
```diff
@@ -45 +45 @@ class InvalidateTest(BitcoinTestFramework):
- tip = int(self.nodes[0].getbestblockhash(), 16)
+ tip = self.nodes[0].getbestblockhash()
@@ -47 +47 @@ class InvalidateTest(BitcoinTestFramework):
- block = create_block(tip, create_coinbase(self.nodes[0].getblockcount()), block_time, version=4)
+ block = create_block(int(tip, 16), create_coinbase(self.nodes[0].getblockcount()), block_time, version=
...
💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2295379703)
> Can you explain this a bit more? I think optimizing the test framework to cleanly shut down on a failure is a bit premature, and may not be needed.
As long as the additional complexity and maintenance burden is low, I think it is worth having a cleaner shutdown to avoid muddying the waters for people trying to figure out root causes.
> However, if a failing test may leave behind a dangling bitcoind process, this seems like something to fix, because it will otherwise lead to test warnings
...
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2295379703)
> Can you explain this a bit more? I think optimizing the test framework to cleanly shut down on a failure is a bit premature, and may not be needed.
As long as the additional complexity and maintenance burden is low, I think it is worth having a cleaner shutdown to avoid muddying the waters for people trying to figure out root causes.
> However, if a failing test may leave behind a dangling bitcoind process, this seems like something to fix, because it will otherwise lead to test warnings
...
🤔 tdb3 reviewed a pull request: "addrman: change internal id counting to int64_t"
(https://github.com/bitcoin/bitcoin/pull/30568#pullrequestreview-2244390291)
Approach ACK
Reviewed a bit more. Just nits.
(https://github.com/bitcoin/bitcoin/pull/30568#pullrequestreview-2244390291)
Approach ACK
Reviewed a bit more. Just nits.
💬 tdb3 commented on pull request "addrman: change internal id counting to int64_t":
(https://github.com/bitcoin/bitcoin/pull/30568#discussion_r1721050883)
nit: The return type in the Doxygen content could be adjusted.
```diff
- * @return int The nid of the entry. If the addrman position is empty or not found, returns -1.
+ * @return nid_type The nid of the entry. If the addrman position is empty or not found, returns -1.
* */
nid_type GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
```
nit / thinking out loud: Returning `-1` is a remnant of the previous type being a pla
...
(https://github.com/bitcoin/bitcoin/pull/30568#discussion_r1721050883)
nit: The return type in the Doxygen content could be adjusted.
```diff
- * @return int The nid of the entry. If the addrman position is empty or not found, returns -1.
+ * @return nid_type The nid of the entry. If the addrman position is empty or not found, returns -1.
* */
nid_type GetEntry(bool use_tried, size_t bucket, size_t position) const EXCLUSIVE_LOCKS_REQUIRED(cs);
```
nit / thinking out loud: Returning `-1` is a remnant of the previous type being a pla
...
📝 andrewtoth opened a pull request: "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries"
(https://github.com/bitcoin/bitcoin/pull/30673)
Following up from https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1655477509, which suggested a revival of #18746.
`GetCoin` will never return `true` for a spent entry, so we can safely assume that any entry we fetch will not be spent. This lets us remove the only non-test code path which adds a FRESH-but-not-DIRTY entry to the flagged linked list. This in turn ensures all entries being sent to `BatchWrite` are `DIRTY` entries.
This is a pure refactor removing dead code which han
...
(https://github.com/bitcoin/bitcoin/pull/30673)
Following up from https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1655477509, which suggested a revival of #18746.
`GetCoin` will never return `true` for a spent entry, so we can safely assume that any entry we fetch will not be spent. This lets us remove the only non-test code path which adds a FRESH-but-not-DIRTY entry to the flagged linked list. This in turn ensures all entries being sent to `BatchWrite` are `DIRTY` entries.
This is a pure refactor removing dead code which han
...
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074014)
Done in #30673 30673
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074014)
Done in #30673 30673
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074195)
Revived in #30673
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074195)
Revived in #30673
💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074348)
Done in #30673
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1721074348)
Done in #30673
💬 gmaxwell commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2295433373)
@vasild I don't think ariaid's initial justification for the PR was the most important one. Sometimes people propose doing the right things for less than the most important reasons. If one were to judge things only by their initial proposals then it would open a very easy attack on development-- just keep proposing good things with bad justification to make sure they won't get done. :)
The amount of CPU-work per byte sent might only be increased by 50% by forcing INV use. E.g. say an at
...
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2295433373)
@vasild I don't think ariaid's initial justification for the PR was the most important one. Sometimes people propose doing the right things for less than the most important reasons. If one were to judge things only by their initial proposals then it would open a very easy attack on development-- just keep proposing good things with bad justification to make sure they won't get done. :)
The amount of CPU-work per byte sent might only be increased by 50% by forcing INV use. E.g. say an at
...
💬 gmaxwell commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2295439986)
Private broadcast can also reduce privacy. I think the weakness is a tossup vs the obvious alternative (run ordinary connections exclusively over Tor) so it's not a reason to not implement the feature, but it's worth discussing in order to understand the limits and see if mitigations are possible.
The issue on my mind is that Tor has little to no resistance to traffic analysis. The small amount of resistance it has is by quantizing messages into fixed size cells. Otherwise, it essentially
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2295439986)
Private broadcast can also reduce privacy. I think the weakness is a tossup vs the obvious alternative (run ordinary connections exclusively over Tor) so it's not a reason to not implement the feature, but it's worth discussing in order to understand the limits and see if mitigations are possible.
The issue on my mind is that Tor has little to no resistance to traffic analysis. The small amount of resistance it has is by quantizing messages into fixed size cells. Otherwise, it essentially
...
✅ willcl-ark closed an issue: "doc: deduplicate list of chain/network strings in RPC/parameter help texts"
(https://github.com/bitcoin/bitcoin/issues/30645)
(https://github.com/bitcoin/bitcoin/issues/30645)
💬 willcl-ark commented on issue "doc: deduplicate list of chain/network strings in RPC/parameter help texts":
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2295837411)
Closing since https://github.com/bitcoin/bitcoin/pull/30648 was merged
(https://github.com/bitcoin/bitcoin/issues/30645#issuecomment-2295837411)
Closing since https://github.com/bitcoin/bitcoin/pull/30648 was merged
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317181)
Taken (mostly).
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317181)
Taken (mostly).
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317268)
Done
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721317268)
Done
💬 Sjors commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange and drop CRPCSignals":
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721324596)
Indeed that's nicer.
(https://github.com/bitcoin/bitcoin/pull/30409#discussion_r1721324596)
Indeed that's nicer.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721360325)
I guess we could add an interim `HexLiteralU`, but that would decrease the motivation to adapt to `std::byte`. :)
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721360325)
I guess we could add an interim `HexLiteralU`, but that would decrease the motivation to adapt to `std::byte`. :)
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721355157)
Yeah, when it came to short/empty hex strings I've been tempted to use something like `std::vector{0xFF}` but felt inconsistent. `valtype` is a good find, but doesn't provide so much of a win once we have full `std::array<std::byte>` support in `CSscript`:
``valtype{0xFF}`` vs
``HexLiteral("FF")``
``valtype{}`` vs
``HexLiteral("")``
(I think switching to base 10 is or using `valtype(36, 0xff);` is too inconsistent). So keeping as-is for now.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721355157)
Yeah, when it came to short/empty hex strings I've been tempted to use something like `std::vector{0xFF}` but felt inconsistent. `valtype` is a good find, but doesn't provide so much of a win once we have full `std::array<std::byte>` support in `CSscript`:
``valtype{0xFF}`` vs
``HexLiteral("FF")``
``valtype{}`` vs
``HexLiteral("")``
(I think switching to base 10 is or using `valtype(36, 0xff);` is too inconsistent). So keeping as-is for now.
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721337019)
I would be happy if you took the ball on that. But would appreciate if you worked on it in a personal branch (still pushing to GitHub for CI) and waited a few days more with making it into a PR.
~6 pushes ago on this PR I had commit 5be34598c4683b2b44f607b28592a9e68e089761 which added targeted `std::array` support to **script.h**. It is conservative in that it doesn't publicly expose the possibility of appending raw `std::span`. IIRC MSVC had some issue with it, but could have been a differen
...
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721337019)
I would be happy if you took the ball on that. But would appreciate if you worked on it in a personal branch (still pushing to GitHub for CI) and waited a few days more with making it into a PR.
~6 pushes ago on this PR I had commit 5be34598c4683b2b44f607b28592a9e68e089761 which added targeted `std::array` support to **script.h**. It is conservative in that it doesn't publicly expose the possibility of appending raw `std::span`. IIRC MSVC had some issue with it, but could have been a differen
...
💬 hodlinator commented on pull request "refactor: Replace ParseHex with consteval HexLiteral":
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721367896)
These lines are part of the scripted diff commit (67fc994bedf14e360b3e51fa1a71dc6c1684b532), and I'd rather not complicate the regexps just for that.
(https://github.com/bitcoin/bitcoin/pull/30377#discussion_r1721367896)
These lines are part of the scripted diff commit (67fc994bedf14e360b3e51fa1a71dc6c1684b532), and I'd rather not complicate the regexps just for that.