💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600099780)
Hmm I see, as per this comment it says "not dependent on any other transactions in the **mempool**" - does this also apply to other transactions in the **block**?
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L613
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1600099780)
Hmm I see, as per this comment it says "not dependent on any other transactions in the **mempool**" - does this also apply to other transactions in the **block**?
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ece11dba3/src/policy/fees.cpp#L613
👍 hebasto approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2055510319)
ACK 850c73e250fe0b248cae80d26561da7047696e3d.
Thanks for the `test/util/check-deps.sh` script!
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2055510319)
ACK 850c73e250fe0b248cae80d26561da7047696e3d.
Thanks for the `test/util/check-deps.sh` script!
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600117347)
Duplicated lines?
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600117347)
Duplicated lines?
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600118750)
```suggestion
**Dependency graph**. Arrows show linker symbol dependencies. *Crypto* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus, crypto and util.
```
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600118750)
```suggestion
**Dependency graph**. Arrows show linker symbol dependencies. *Crypto* lib depends on nothing. *Util* lib is depended on by everything. *Kernel* lib depends only on consensus, crypto and util.
```
💬 hebasto commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600120289)
nit: Sort these lines?
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1600120289)
nit: Sort these lines?
💬 ryanofsky commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2110376536)
> Maybe I could start by having the `getblocktemplate` RPC use that new interface?
That's a good idea, nice way the interface could be used from the start.
> And then interface implementation would take care of getting the chain manager and mempool (via the node interface?).
Right, you could look at the interfaces::Chain for an example of how to initialize the interface. Store the interface in NodeContext like:
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ec
...
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-2110376536)
> Maybe I could start by having the `getblocktemplate` RPC use that new interface?
That's a good idea, nice way the interface could be used from the start.
> And then interface implementation would take care of getting the chain manager and mempool (via the node interface?).
Right, you could look at the interfaces::Chain for an example of how to initialize the interface. Store the interface in NodeContext like:
https://github.com/bitcoin/bitcoin/blob/7fcf4e99791ca5be0b068ac03a81a50ec
...
💬 hebasto commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2110387585)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2110387585)
Concept ACK.
💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110388198)
> Maybe this is starting to be more complicated than just having a struct per each reason? But I'd still argue this is a better approach in that it keeps all the logic for mempool removal reasons in one place and avoids duplicating code on each struct.
Same thing I was thinking. Using the class right now looks like it will make things more complicated. Maybe we should leave the class for a future change where it becomes necessary? @josibake @ryanofsky
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110388198)
> Maybe this is starting to be more complicated than just having a struct per each reason? But I'd still argue this is a better approach in that it keeps all the logic for mempool removal reasons in one place and avoids duplicating code on each struct.
Same thing I was thinking. Using the class right now looks like it will make things more complicated. Maybe we should leave the class for a future change where it becomes necessary? @josibake @ryanofsky
👍 hebasto approved a pull request: "kernel: Remove batchpriority from kernel library"
(https://github.com/bitcoin/bitcoin/pull/30083#pullrequestreview-2055622041)
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, I have reviewed the code and it looks OK.
(https://github.com/bitcoin/bitcoin/pull/30083#pullrequestreview-2055622041)
ACK d4b17c7d46ad8e2833ade99d5b4c9741c913e84d, I have reviewed the code and it looks OK.
💬 ryanofsky commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110436529)
> Seems easily addressed with a constructor, no? Something like:
Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition. Depending on the constructors it may also only provide runtime checking rather than compile-time checking like in your example. And if the struct is mutable could allow invalid representations of state after construction.
I don't know what is best in this particular case, I would ju
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110436529)
> Seems easily addressed with a constructor, no? Something like:
Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition. Depending on the constructors it may also only provide runtime checking rather than compile-time checking like in your example. And if the struct is mutable could allow invalid representations of state after construction.
I don't know what is best in this particular case, I would ju
...
💬 josibake commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110464755)
> Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition
Fair point, in that bugs could be introduced by someone not writing these pre-checks correctly / efficiently. I'll admit I'm not fully convinced that the struct per state approach isn't going to be harder to maintain / extend in the future, but given that I haven't convinced you guys on that point and you have convinced me there is an advantage per
...
(https://github.com/bitcoin/bitcoin/pull/29680#issuecomment-2110464755)
> Yes, but those are manual constraints that you are writing by hand rather than automatic constraints expressed implicitly in the data definition
Fair point, in that bugs could be introduced by someone not writing these pre-checks correctly / efficiently. I'll admit I'm not fully convinced that the struct per state approach isn't going to be harder to maintain / extend in the future, but given that I haven't convinced you guys on that point and you have convinced me there is an advantage per
...
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1600207532)
Thanks so much for the patch. I had to move this line back up to where it was, otherwise the test fails because after pushing the error into the batch reply, we would also then push a weird extra junk reply with the current id but the result from the last successful query
```diff
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 0e3e66c515..777ad32bbe 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -238,12 +238,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPReque
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1600207532)
Thanks so much for the patch. I had to move this line back up to where it was, otherwise the test fails because after pushing the error into the batch reply, we would also then push a weird extra junk reply with the current id but the result from the last successful query
```diff
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
index 0e3e66c515..777ad32bbe 100644
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -238,12 +238,12 @@ static bool HTTPReq_JSONRPC(const std::any& context, HTTPReque
...
🤔 hebasto reviewed a pull request: "util: avoid using thread_local variable that has a destructor"
(https://github.com/bitcoin/bitcoin/pull/30095#pullrequestreview-2055708177)
Approach ACK c5f9afd946efb4eb6b27b2d0316f2f787a9608c7.
(https://github.com/bitcoin/bitcoin/pull/30095#pullrequestreview-2055708177)
Approach ACK c5f9afd946efb4eb6b27b2d0316f2f787a9608c7.
💬 hebasto commented on pull request "util: avoid using thread_local variable that has a destructor":
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1600218912)
Maybe add a comment to prevent future attempts to reintroduce `std::string`?
(https://github.com/bitcoin/bitcoin/pull/30095#discussion_r1600218912)
Maybe add a comment to prevent future attempts to reintroduce `std::string`?
💬 setavenger commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1600219420)
Took a bit longer than I expected. I've pushed to the [dev branch](https://github.com/setavenger/blindbit-oracle/tree/dev). It seems to be working as intended. No cut-through and you can choose any value you like for a dustLimit. The request looks like this `"localhost:8000/tweak-index/840000?dustLimit=546"` (`tweak-index` then only returns tweaks where the highest value is `>= 546`)
(https://github.com/bitcoin/bitcoin/pull/28241#discussion_r1600219420)
Took a bit longer than I expected. I've pushed to the [dev branch](https://github.com/setavenger/blindbit-oracle/tree/dev). It seems to be working as intended. No cut-through and you can choose any value you like for a dustLimit. The request looks like this `"localhost:8000/tweak-index/840000?dustLimit=546"` (`tweak-index` then only returns tweaks where the highest value is `>= 546`)
💬 pinheadmz commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1600225849)
I'm trying to figure out how to write a test to catch one of these `RPC_MISC_ERROR` since they obviously weren't catching a `RPC_PARSE_ERROR` before adding your patch to the commit. If I understand correctly, that "misc" error would only throw if there was a bug in an RPC command because most of our try blocks try to catch a UniValue error first with a specific code.
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1600225849)
I'm trying to figure out how to write a test to catch one of these `RPC_MISC_ERROR` since they obviously weren't catching a `RPC_PARSE_ERROR` before adding your patch to the commit. If I understand correctly, that "misc" error would only throw if there was a bug in an RPC command because most of our try blocks try to catch a UniValue error first with a specific code.
💬 glozow commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600233020)
(since @sr-gi mentioned this offline) Yes, the last push turned this 1 line into 2 identical lines. The purpose is to illustrate to current reviewers and future readers more clearly... if wtxid, we do this. if txid, we also do this, but notice that we are actually doing a "cast" and that's ok because we are *guessing* what the wtxid is.
Also, perhaps these will diverge again in the future if `GenTxid` becomes a `std::variant<Txid, Wtxid>`.
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1600233020)
(since @sr-gi mentioned this offline) Yes, the last push turned this 1 line into 2 identical lines. The purpose is to illustrate to current reviewers and future readers more clearly... if wtxid, we do this. if txid, we also do this, but notice that we are actually doing a "cast" and that's ok because we are *guessing* what the wtxid is.
Also, perhaps these will diverge again in the future if `GenTxid` becomes a `std::variant<Txid, Wtxid>`.
🚀 ryanofsky merged a pull request: "kernel: Remove batchpriority from kernel library"
(https://github.com/bitcoin/bitcoin/pull/30083)
(https://github.com/bitcoin/bitcoin/pull/30083)
🤔 edilmedeiros reviewed a pull request: "Extend signetchallenge to set target block spacing"
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2055713496)
Concept ACK.
I prefer the approach in #27446 but I can't say this would not be the bitcoin style of using the existing script machinery to implement so a logic.
I would love to see an accompanying BIP to upgrade BIP 325, since the signet behavior is standardized there. In practice, this change in core will potentially make this implementation an "undocumented standard".
I personally would prefer the BIP discussion to happen before merging, specially to define which params would be incl
...
(https://github.com/bitcoin/bitcoin/pull/29365#pullrequestreview-2055713496)
Concept ACK.
I prefer the approach in #27446 but I can't say this would not be the bitcoin style of using the existing script machinery to implement so a logic.
I would love to see an accompanying BIP to upgrade BIP 325, since the signet behavior is standardized there. In practice, this change in core will potentially make this implementation an "undocumented standard".
I personally would prefer the BIP discussion to happen before merging, specially to define which params would be incl
...
💬 edilmedeiros commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1600222312)
I don't feel like changing this test is a good idea since the proposed change is supposed to be backward-compatible. Better would be to add an additional functional test just for the purpose of testing the new behavior while keeping the old functional test as a guard rail.
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r1600222312)
I don't feel like changing this test is a good idea since the proposed change is supposed to be backward-compatible. Better would be to add an additional functional test just for the purpose of testing the new behavior while keeping the old functional test as a guard rail.